Skip to content

source_basis/module_ao: orb_atomic_lm_test off #define private public, via existing accessors plus one friend - #7953

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/orb-lm-tests-friend
Sep 12, 2026
Merged

mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/orb-lm-tests-friend

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

Continues the #define private public cleanup (#7940, #7949, #7952). This one
takes orb_atomic_lm_test.cpp, the largest single concentration of the pattern
left in the tree.

What the macro was covering

The file reaches into 17 private members of Numerical_Orbital_Lm and calls
four of its private methods. No PARAM is involved. Sixteen of the seventeen
members 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 each
accessor returns the member itself: get_psi() yields
const std::vector<double>&, so .psi[i], .psi.size(), .psi.empty() and a
bare .psi all keep working through it, and the scalar getters return const
references. 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 one

Numerical_Orbital_Lm exposes a systematic trio for each array:

const double*  getPsi() const                 { ... }   // pointer
const double&  getPsi(const int ir) const     { ... }   // element
const std::vector<double>& get_psi() const    { ... }   // vector

psi, psif, psik, psik2, r_radial and rab all have all three. psir
had 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 obvious
gap 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_sbpool and plot are 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 the friend class Numerical_Orbital; the class already carries.

Friendship is not inherited and a TEST_F body lives in a class derived from
the fixture, so the fixture gained four forwarding wrappers and the bodies call
those — the arrangement already used by dftu_lcao_test.cpp and introduced for
this cleanup in #7949.

Why not simply grant friendship for everything

A friend would have made all 138 sites compile untouched, and the diff would
have 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

before after
files with the macro (tree-wide) 63 62
occurrences 91 90
private members of Numerical_Orbital_Lm named in a TEST_F body 17 0

Production 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 — the module_ao tests take their orbital data
from install(DIRECTORY lcao_H2O ...), so without that step ORB_read_test
fails and ORB_atomic_lm_test / ORB_nonlocal_lm_test segfault on missing
input, both before and after this change.

  • build: 0 errors.
  • MODULE_AO_ORB_atomic_lm_test and the three sibling module_ao tests: all pass.
  • full unit suite: 2 of 339 fail, the same two as upstream/develop
    (MODULE_HSOLVER_diago_hs_parallel, MODULE_HSOLVER_LCAO, both
    mpirun-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/GlobalC reference is added or removed.
  • After the rewrite, checked mechanically that no private member of
    Numerical_Orbital_Lm is named anywhere in a TEST_F body, and that the file
    contains no std::swap, address-of or assignment form that would have slipped
    past a read-only substitution.

No INPUT parameter and no user-visible behaviour changed, so
docs/parameters.yaml and docs/advanced/input_files/input-main.md need no
update.

What is not here

orb_nonlocal_lm_test.cpp is the last module_ao file carrying the macro. It
needs 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 private
get_kradial() to check round-trip consistency — and asserts on raw pointers
being nulled by freemem() / restored by renew(). That needs mutating
friend 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

…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>

@mohanchen mohanchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Sep 12, 2026
@mohanchen
mohanchen merged commit afb52a3 into deepmodeling:develop Sep 12, 2026
17 checks passed
@Critsium-xy
Critsium-xy deleted the refactor/orb-lm-tests-friend branch September 14, 2026 05:25
Critsium-xy added a commit to Critsium-xy/abacus-develop that referenced this pull request Sep 14, 2026
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 pushed a commit that referenced this pull request Sep 14, 2026
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.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 #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 #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 #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 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants