source_basis/module_ao: remove the last #define private public in the module - #7965
Merged
mohanchen merged 1 commit intoSep 14, 2026
Merged
Conversation
… plus one friend `orb_nonlocal_lm_test.cpp` is the last file in `module_ao` carrying `#define private public`. It reaches 78 times into `Numerical_Nonlocal_Lm` from its TEST_F bodies. Most of that needs nothing from the class: ten of the members already have public accessors, so `nnl[ip].nr` becomes `nnl[ip].getNr()`, `.r_radial[ir]` becomes `.getRadial(ir)`, `.beta_k[ik]` becomes `.getBeta_k(ik)`, and so on. Six things have no public route and get `friend class NumericalNonlocalLmTest;` plus thin read-only forwarders on the fixture: the members `label`, `kcut`, `index_proj` and `rab`, and the private methods `freemem()` and `renew()`, which the FreeAndRenew test exists to exercise. Friendship is not inherited and a TEST_F body lives in a derived class, hence the forwarders -- the arrangement established in deepmodeling#7949. Nothing that mutates state is exposed. The r->k->r round-trip check does reach deep -- it swaps the r-space and k-space arrays of a projector, reallocates `rab`, and calls the private `get_kradial()` -- but that code lives in `NumericalNonlocalLmTest::err_r2k2r`, a member of the fixture itself, so the friend declaration covers it and it is unchanged. 28 of the 106 total accesses are inside fixture helpers like that one and needed no edit. No test expectation changed; every substitution reads the same member through the accessor that returns it. With this, `module_ao` has no `#define private public` left. Occurrences tree-wide 84 -> 83. Production change is four lines: a forward declaration and one friend. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohanchen
added
Refactor
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
2 participants
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_basis/module_ao: last access hack in the module, via accessors plus one friend
orb_nonlocal_lm_test.cppis the last file inmodule_aocarrying#define private public. It reaches 78 times intoNumerical_Nonlocal_Lmfromits TEST_F bodies. Most of that needs nothing from the class: ten of the members
already have public accessors, so
nnl[ip].nrbecomesnnl[ip].getNr(),.r_radial[ir]becomes.getRadial(ir),.beta_k[ik]becomes.getBeta_k(ik), and so on.Six things have no public route and get
friend class NumericalNonlocalLmTest;plus thin read-only forwarders on the fixture: the members
label,kcut,index_projandrab, and the private methodsfreemem()andrenew(), whichthe FreeAndRenew test exists to exercise. Friendship is not inherited and a
TEST_F body lives in a derived class, hence the forwarders -- the arrangement
established in #7949.
Nothing that mutates state is exposed. The r->k->r round-trip check does reach
deep -- it swaps the r-space and k-space arrays of a projector, reallocates
rab, and calls the privateget_kradial()-- but that code lives inNumericalNonlocalLmTest::err_r2k2r, a member of the fixture itself, so thefriend declaration covers it and it is unchanged. 28 of the 106 total accesses
are inside fixture helpers like that one and needed no edit.
No test expectation changed; every substitution reads the same member through
the accessor that returns it. With this,
module_aohas no#define private publicleft. Occurrences tree-wide 84 -> 83.Production change is four lines: a forward declaration and one friend.
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, thencmake --install build(themodule_aotests take their orbital data frominstall(DIRECTORY lcao_H2O ...)):source_lcao/module_lr/.../lr_io_krlist.cpp.o, onri_util.h: RI/global/Array_Operator.h: No such file or directory. That is pre-existing ondevelopin 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_aotests pass:MODULE_AO_ORB_nonlocal_test,MODULE_AO_ORB_nonlocal_lm_test,MODULE_AO_ORB_atomic_lm_test,MODULE_AO_ORB_read_test.agent_governance_check.py --base upstream/develop --head HEAD: 0 errors. The access-hack ratchet reports nothing (1 removed, 0 added), and noPARAM/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