source_basis/module_ao: orb_atomic_lm_test off #define private public, via existing accessors plus one friend - #7953
Merged
mohanchen merged 1 commit intoSep 12, 2026
Conversation
…etters plus one friend `orb_atomic_lm_test.cpp` reached into 17 private members of `Numerical_Orbital_Lm` and called four of its private methods. Sixteen of those members already have public accessors, so the macro comes off mostly by using them: 138 sites now read through `get_psi()`, `getNr()`, `getRcut()` and the rest. No PARAM is involved. Two things needed more than a substitution: - `psir` was the only array in the class without the vector accessor its siblings all have. `Numerical_Orbital_Lm` exposes a systematic trio per array -- `getX()` returning a pointer, `getX(i)` returning an element, `get_x()` returning the vector -- and psir had only the first two, so `.psir.size()` and `.psir.empty()` had no public route. Added the missing `get_psir()`, completing the pattern rather than inventing an accessor for the test. - `cal_kradial`, `cal_kradial_sbpool`, `cal_rradial_sbpool` and `plot` are private and are what four of the tests exist to exercise. These get `friend class NumericalOrbitalLmTest;`, next to the `friend class Numerical_Orbital;` the class already carries, plus four forwarding wrappers on the fixture -- a TEST_F body lives in a derived class and does not inherit friendship. The substitution is uniform because the accessors return the member itself: `get_psi()` yields `const std::vector<double>&`, so `.psi[i]`, `.psi.size()`, `.psi.empty()` and bare `.psi` all keep working through it, and the scalar getters return const references. Every site was rewritten mechanically and then checked: no private member of the class is named in any TEST_F body any more. No test expectation changed. Macro occurrences in this file go 1 -> 0; no `#undef private` is added and no other file is touched. 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.
Continues the
#define private publiccleanup (#7940, #7949, #7952). This onetakes
orb_atomic_lm_test.cpp, the largest single concentration of the patternleft in the tree.
What the macro was covering
The file reaches into 17 private members of
Numerical_Orbital_Lmand callsfour of its private methods. No
PARAMis involved. Sixteen of the seventeenmembers already have public accessors, so most of the macro's job was to let
the test bypass an interface that was already there.
138 sites now read through those accessors —
get_psi(),getNr(),getRcut(),getL()and the rest. The substitution is uniform because eachaccessor returns the member itself:
get_psi()yieldsconst std::vector<double>&, so.psi[i],.psi.size(),.psi.empty()and abare
.psiall keep working through it, and the scalar getters return constreferences. Nothing about what the test asserts changes.
Two things needed more than a substitution, and they are deliberately handled
differently.
psir— completing an accessor set, not inventing oneNumerical_Orbital_Lmexposes a systematic trio for each array:psi,psif,psik,psik2,r_radialandraball have all three.psirhad only the pointer and element forms, so
.psir.size()and.psir.empty()had no public route. This adds the missing
get_psir(), which fills an obviousgap in the class's own pattern rather than adding an accessor because a test
wanted one.
The four private methods — a named friend grant
cal_kradial,cal_kradial_sbpool,cal_rradial_sbpoolandplotare private,and exercising them is precisely why four of these tests exist (the file header
lists them under "Tested functions"). They get
friend class NumericalOrbitalLmTest;, placed next to thefriend class Numerical_Orbital;the class already carries.Friendship is not inherited and a
TEST_Fbody lives in a class derived fromthe fixture, so the fixture gained four forwarding wrappers and the bodies call
those — the arrangement already used by
dftu_lcao_test.cppand introduced forthis cleanup in #7949.
Why not simply grant friendship for everything
A
friendwould have made all 138 sites compile untouched, and the diff wouldhave been a dozen lines. It would also have left the test reaching past an
interface that already exposes exactly what it needs. The narrower reading is
that friendship is for what genuinely has no public route — here, four private
methods — and the accessors are for everything else.
Result
Numerical_Orbital_Lmnamed in a TEST_F bodyProduction change is 4 lines: one accessor and one friend declaration.
Verification
Linux,
cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON,then
cmake --install build— themodule_aotests take their orbital datafrom
install(DIRECTORY lcao_H2O ...), so without that stepORB_read_testfails and
ORB_atomic_lm_test/ORB_nonlocal_lm_testsegfault on missinginput, both before and after this change.
MODULE_AO_ORB_atomic_lm_testand the three siblingmodule_aotests: all pass.upstream/develop(
MODULE_HSOLVER_diago_hs_parallel,MODULE_HSOLVER_LCAO, bothmpirun-based). No new failures.agent_governance_check.py --base upstream/develop --head HEAD: 0 errors.The access-hack ratchet reports nothing (1 removed, 0 added), and no
PARAM/GlobalV/GlobalCreference is added or removed.Numerical_Orbital_Lmis named anywhere in aTEST_Fbody, and that the filecontains no
std::swap, address-of or assignment form that would have slippedpast a read-only substitution.
No INPUT parameter and no user-visible behaviour changed, so
docs/parameters.yamlanddocs/advanced/input_files/input-main.mdneed noupdate.
What is not here
orb_nonlocal_lm_test.cppis the lastmodule_aofile carrying the macro. Itneeds a different treatment and is left for a separate PR: besides the read-only
substitutions, it deliberately mutates internals — swapping the r-space and
k-space arrays of a projector, reallocating
rab, then calling the privateget_kradial()to check round-trip consistency — and asserts on raw pointersbeing nulled by
freemem()/ restored byrenew(). That needs mutatingfriend wrappers, which is a different review question from the read-only
forwarding here, and is better expressed as one wrapper for the whole
r-to-k swap than as per-field access.
🤖 Generated with Claude Code