source_cell / source_pw: remove four more #define private public - #7963
Merged
mohanchen merged 1 commit intoSep 14, 2026
Merged
Conversation
Critsium-xy
force-pushed
the
refactor/access-hack-small-batch
branch
2 times, most recently
from
September 14, 2026 07:44
74f7fbc to
7cc03aa
Compare
Continues the cleanup (deepmodeling#7940, deepmodeling#7949, deepmodeling#7952, deepmodeling#7953). Four macros come off, and in three of the four files nothing is granted to anyone. - `read_sep_test.cpp` is vestigial: it carries the macro but touches nothing private in `sep.h`. The directives are deleted and nothing else changes, as with `cal_test.cpp` and `test_hsolver.cpp` in deepmodeling#7921. - `soc_test.cpp` read `soc.p_rot[l2p1*i + n]` five times, while `Soc` has a public `rotylm(i1, i2)` returning exactly `p_rot[l2plus1_*i1 + i2]`. Those become `soc.rotylm(i, n)` / `soc.rotylm(i+1, n)`. The sixth use was `EXPECT_NE(soc.p_rot, nullptr)`. That assertion is dropped rather than kept alive with a friend declaration: the next line already calls `soc.rotylm(0, 0)` and checks its value, which covers both "was it allocated" and "is it correct", and the guard was `EXPECT_` rather than `ASSERT_`, so it did not even stop the dereference that follows. `soc.h` is therefore untouched by this PR. - `atom_spec_test.cpp` calls the private `Pseudopot_upf::read_pseudo_upf201`, which writes thirteen members of its object and has no stateless form. It gets `friend class AtomSpecTest;` alongside the `AtomPseudoTest`, `NCPPTest` and `ReadPPTest` grants already there from deepmodeling#7949, plus a forwarding wrapper. The `atom.type` and `atom.ncpp` accesses in the same file are public members of `Atom` and never needed the macro. - `sltk_grid_test.cpp` wrote `PARAM.input.test_grid = 1` only to pass it into `Grid LatGrid(PARAM.input.test_grid)` -- PARAM used as a local, so it becomes one. Its genuine private access is `Grid::setMemberVariables`, 117 lines setting members from a UnitCell, so that gets `friend class SltkGridTest;` and a wrapper. `Grid::pbc` and `Grid::sradius2`, also read here, are public. Friendship is not inherited and a TEST_F body lives in a derived class, hence the wrappers -- the arrangement established in deepmodeling#7949. An earlier revision also removed the macro from `memory_test.cpp` and the three `propagator_test*.cpp`. Compiling without it showed both readings wrong: `memory_test` reads `Memory::name`, `class_name`, `consume` and `init_flag`, private statics reached through `::` rather than a member access; the propagator tests read `PARAM.input`, so they are reason-(a) work. Both are left alone, per the rule this series follows: remove the macro, or leave the file untouched. Macro occurrences 88 -> 84. Production change is two `friend` declarations, one of them added to a list that already exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohanchen
approved these changes
Sep 14, 2026
mohanchen
pushed a commit
that referenced
this pull request
Sep 14, 2026
…public (#7966) Continues the cleanup (#7940, #7949, #7952, #7953, and open #7963/#7964/#7965). Two class families this time, handled by the same triage: use the accessor that exists, and grant friendship only for what genuinely has no public route. Measured against the TEST_F bodies -- accesses inside fixture members need nothing, since the fixture is the friend -- the four files touch far less than their size suggests: 10, 6, 5 and 1 sites respectively. - **`pw_basis_k_test.cpp` needs nothing granted.** Its only non-public reads are `device` and `precision`, and `PW_Basis` already has `get_device()` / `get_precision()`. - **`pw_basis_test.cpp`** reads the same two through those accessors, and calls three protected routines -- `distribute_g()`, `distribute_r()` and `getstartgr()` -- which set 7, 28 and 33 members of their object and have no stateless form. Those get `friend class ::PWBasisTEST;` and three forwarders. - **`test_hsolver_pw.cpp`** has exactly one live call into protected territory, `hamiltSolvePsiK` (30 `this->`), in the NpwxLessThanNbandsDeath test; the other references to it and to `update_precondition` are commented out. It gets `friend class ::TestHSolverPW;` and one forwarder. - **`test_hsolver_sdft.cpp`** is vestigial: every `TEST_F` in it is commented out, and the `nbands` it appeared to touch is `stowf.nbands_diag`, a member of a different class. The directives are simply deleted, as with `cal_test.cpp` and `test_hsolver.cpp` in #7921. `FFT_Bundle` gains `get_device()` and `get_precision()`. The pw tests check that `PW_Basis`'s constructor propagates device and precision into its `fft_bundle`, which is a real behavioural check and not redundant, but `FFT_Bundle`'s copies were private with no accessor. `PW_Basis` already exposes exactly this pair, so this completes a parallel that was half-present rather than inventing an accessor for a test. Both friended classes live in a namespace (`ModulePW`, `hsolver`) while the fixtures are at global scope, so each needs a global forward declaration and `friend class ::Fixture;` -- an unqualified `friend class Fixture;` would name a nonexistent class inside the namespace and silently grant nothing. Occurrences tree-wide 88 -> 84. Production change is three friend declarations with their forward declarations, plus the two `FFT_Bundle` accessors. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
source_cell / source_pw: remove four more #define private public
Continues the cleanup (#7940, #7949, #7952, #7953). Four macros come off, and
in three of the four files nothing is granted to anyone.
read_sep_test.cppis vestigial: it carries the macro but touches nothingprivate in
sep.h. The directives are deleted and nothing else changes, aswith
cal_test.cppandtest_hsolver.cppin source_relax: pass INPUT values explicitly instead of reading PARAM, removing the test access hacks it caused #7921.soc_test.cppreadsoc.p_rot[l2p1*i + n]five times, whileSochas apublic
rotylm(i1, i2)returning exactlyp_rot[l2plus1_*i1 + i2]. Thosebecome
soc.rotylm(i, n)/soc.rotylm(i+1, n).The sixth use was
EXPECT_NE(soc.p_rot, nullptr). That assertion is droppedrather than kept alive with a friend declaration: the next line already calls
soc.rotylm(0, 0)and checks its value, which covers both "was it allocated"and "is it correct", and the guard was
EXPECT_rather thanASSERT_, so itdid not even stop the dereference that follows.
soc.his thereforeuntouched by this PR.
atom_spec_test.cppcalls the privatePseudopot_upf::read_pseudo_upf201,which writes thirteen members of its object and has no stateless form. It
gets
friend class AtomSpecTest;alongside theAtomPseudoTest,NCPPTestand
ReadPPTestgrants already there from source_cell: replace #define private public with named friend grants in four tests #7949, plus a forwarding wrapper.The
atom.typeandatom.ncppaccesses in the same file are public membersof
Atomand never needed the macro.sltk_grid_test.cppwrotePARAM.input.test_grid = 1only to pass it intoGrid LatGrid(PARAM.input.test_grid)-- PARAM used as a local, so it becomesone. Its genuine private access is
Grid::setMemberVariables, 117 linessetting members from a UnitCell, so that gets
friend class SltkGridTest;and a wrapper.
Grid::pbcandGrid::sradius2, also read here, are public.Friendship is not inherited and a TEST_F body lives in a derived class, hence
the wrappers -- the arrangement established in #7949.
An earlier revision also removed the macro from
memory_test.cppand the threepropagator_test*.cpp. Compiling without it showed both readings wrong:memory_testreadsMemory::name,class_name,consumeandinit_flag,private statics reached through
::rather than a member access; the propagatortests read
PARAM.input, so they are reason-(a) work. Both are left alone, perthe rule this series follows: remove the macro, or leave the file untouched.
Macro occurrences 88 -> 84. Production change is two
frienddeclarations, oneof them added to a list that already exists.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Verification
Linux,
cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON:source_lcao/module_lr/.../lr_io_krlist.cpp.o,on
ri_util.h: RI/global/Array_Operator.h: No such file or directory. That ispre-existing on
developin an environment without LibRI headers (from Refactor LR_IO function and add test case 58_KP_LR_BSE #7849);this PR touches neither file.
MODULE_CELL_NEIGHBOR_sltk_grid,MODULE_CELL_atom_spec,MODULE_CELL_read_sep_parallel,MODULE_CELL_sep_cell_parallel,MODULE_PW_pwdft_soc.agent_governance_check.py --base upstream/develop --head HEAD: 0 errors. Theaccess-hack ratchet reports nothing (4 removed, 0 added), and no
PARAM/GlobalV/GlobalCreference is added or removed.No INPUT parameter and no user-visible behaviour changed, so
docs/parameters.yamlanddocs/advanced/input_files/input-main.mdneed no update.🤖 Generated with Claude Code