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

source_basis/module_ao: orb_atomic_lm_test off #define private public, via existing accessors plus one friend#7953
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
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