Skip to content

source_cell: replace #define private public with named friend grants in four tests - #7949

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/pseudo-magnetism-friend-tests
Sep 10, 2026
Merged

mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/pseudo-magnetism-friend-tests

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

Second tranche of the #define private public cleanup, after #7940. That PR
removed the macros whose cause was the code under test reading global PARAM;
this one removes the macros whose cause is the test calling a private method,
which needs a different fix and touches no global state at all.

Which files, and why these four

Classifying every macro region in the tree by what it actually covers gives two
root causes — (a) the production code reads global PARAM itself, so the
test can only drive it by writing the private PARAM.input/PARAM.sys; and
(b) the test reaches into the class under test. (b) subdivides further, and
the cheapest sub-case is the one handled here:

the test calls a private method — no private data member is involved.

For these four files that is the only cause. None of them touches PARAM, so
there is no global-dependency work mixed in and no governance budget to balance
(unlike #7940, where the call-site cost had to be paid for by a paired cleanup).

  • magnetism_test.cppMagnetism::judge_parallel, 2 call sites.
  • atom_pseudo_test.cppPseudopot_upf::read_pseudo_upf201, 2 call sites.
  • pseudo_nc_test.cppread_pseudo_upf201, complete_default_h,
    complete_default_atom, 8 call sites.
  • read_pp_test.cppread_pseudo_upf, read_pseudo_upf201,
    read_pseudo_vwr, read_pseudo_blps, set_pseudo_type, setqfnew, trim,
    trimend, 26 call sites.

In every case the private method is the thing the test exists to exercise — the
file headers say so (read_pp_test.cpp lists read_pseudo_upf,
set_pseudo_type, trim/trimend under "Tested Functions", and
magnetism_test.cpp lists Magnetism::judge_parallel()).

The fix

An explicit friend class <fixture>; on the class under test, which is what
AGENTS.md rule 10 names and what this codebase already does in
source_pw/module_pwdft/dftu_base.h (friend class DFTUTest;) and
source_io/module_parameter/parameter.h (friend class TestParameters;). The
whole production-side change is 15 lines:

class MagnetismTest;

class Magnetism
{
    /// @brief the unit test drives the private judge_parallel() helper directly
    friend class MagnetismTest;

public:

plus the same for Pseudopot_upf with its three fixtures. No method changed
visibility, no signature changed, no member was added.

This is a real narrowing rather than a cosmetic one. #define private public
reinterprets access control for the entire translation unit — including every
standard library header the TU pulls in, which is undefined behaviour that
surfaces at link or run time rather than as a compile error — and leaves that TU
disagreeing with the rest of the build about the layout and accessibility of
every class it sees. A friend declaration grants one named class access to one
other class, is visible in the header where the class is defined, and is
reviewable.

Friendship is not inherited and a TEST_F body lives in a class derived from
the fixture, so each fixture gains thin forwarding wrappers and the TEST_F
bodies call those. dftu_lcao_test.cpp already documents and uses exactly this
arrangement. Every wrapper forwards its arguments unmodified and returns what the
private method returns.

What is deliberately unchanged

Public members and public methods the tests already used are untouched and still
accessed directly: Pseudopot_upf::complete_default, init_pseudo_reader,
average_p, set_upf_q, set_empty_element, print_pseudo_upf and its public
data (kbeta, qfunc, qfcoef, lloc, nqf, ...), and Magnetism's
tot_mag / abs_mag / start_mag / compute_mag. Only the calls that actually
needed the macro were rerouted.

Magnetism::judge_parallel could alternatively have been made a public static —
it is a pure predicate that touches no member. That was not done: it would
enlarge the public API permanently to serve a test, where friend does not.

No #undef private is introduced anywhere, and no file whose macro this PR does
not remove is touched — the scope rule from #7921.

Result

before after
files with the macro (tree-wide) 67 63
occurrences 95 91
macro occurrences in these four files 4 0

Verification

Linux, cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON:

  • build: 0 errors (branch and base both BUILD_EXIT=0).
  • the affected tests: MODULE_CELL_read_pp, MODULE_CELL_pseudo_nc,
    MODULE_CELL_atom_pseudo, MODULE_CELL_bcast_atom_pseudo_test,
    MODULE_CELL_magnetismall pass, with every assertion and expected value
    unchanged.
  • full unit suite: 29 of 339 fail — the failure set is identical to the base
    commit
    (d3722debd) built and run in a parallel worktree in the same
    environment; both comm directions are empty. The 29 are pre-existing and
    environment-related (mpirun-based *_para/*_parallel wrappers, plus
    cubic_spline, math_sphbes, dav, bpcg, PSI_init, ...).
  • agent_governance_check.py: 0 errors, exit 0. The access-hack ratchet
    reports nothing (4 removed, 0 added), and the global-dependency budget is
    untouched because no PARAM/GlobalV/GlobalC reference is added or removed.
  • verified via compiled objects that all four changed test files were actually
    built, rather than skipped by the local feature configuration.

The one governance warning is "Documentation sync review": no INPUT parameter and
no user-visible behaviour changed — this is a test-visibility change only — so
docs/parameters.yaml and docs/advanced/input_files/input-main.md need no
update.

What is not here

The remaining 63 files split by the same taxonomy. The next cheapest group is
tests that read a private member for which a public getter already exists and
is simply not being used — those need no production change at all. A related but
distinct case has to be handled with care: where the assertion is
EXPECT_EQ(obj.get_x(), obj.x), substituting the getter makes it a tautology, so
the assertion has to be re-anchored to the known input value instead (which also
makes the test stronger). Beyond that lie tests that read private state with no
getter (a public const observer) and tests that write private state to build
a fixture (a setter or a different construction path) — the latter is the
expensive tail and includes klist_test.cpp, charge_mixing_test.cpp and
read_input_item_test.cpp.

🤖 Generated with Claude Code

…in four tests

`magnetism_test`, `atom_pseudo_test`, `pseudo_nc_test` and `read_pp_test` each
switched off access control for their whole translation unit in order to call a
handful of private *methods* -- the format readers and helpers the tests exist
to exercise. No private data member is involved, and none of these files touches
PARAM, so the macro had exactly one cause.

Replace it with an explicit `friend class <fixture>;` on the class under test,
following the existing precedent in source_pw/module_pwdft/dftu_base.h
(`friend class DFTUTest;`) and parameter.h (`friend class TestParameters;`).
This is the fix AGENTS.md rule 10 names, and it narrows a whole-TU override --
which also reinterprets access control inside every standard library header the
TU pulls in -- down to one named, reviewable grant per fixture.

Friendship is not inherited, and a TEST_F body lives in a class derived from the
fixture, so each fixture gains thin forwarding wrappers and the TEST_F bodies
call those. dftu_lcao_test.cpp already documents and uses this arrangement.

- Magnetism: friend MagnetismTest, 1 wrapper (judge_parallel), 2 call sites.
- Pseudopot_upf: friend AtomPseudoTest / NCPPTest / ReadPPTest; 1 + 3 + 8
  wrappers for read_pseudo_upf{,201,_vwr,_blps}, set_pseudo_type, setqfnew,
  trim, trimend, complete_default_{h,atom}; 36 call sites in total.

Public members and public methods the tests already used -- Pseudopot_upf's
complete_default, init_pseudo_reader, average_p, set_upf_q, set_empty_element,
print_pseudo_upf and its public data, and Magnetism's tot_mag/abs_mag/start_mag
-- are untouched and still accessed directly.

No test expectation changed: every wrapper forwards its arguments unmodified and
returns what the private method returns. Macro occurrences in these four files
go 4 -> 0; no `#undef private` is introduced anywhere, and no file whose macro
survives 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 10, 2026
@mohanchen
mohanchen merged commit 9cbcc0b into deepmodeling:develop Sep 10, 2026
17 checks passed
@Critsium-xy
Critsium-xy deleted the refactor/pseudo-magnetism-friend-tests branch September 14, 2026 05:25
Critsium-xy added a commit to Critsium-xy/abacus-develop that referenced this pull request Sep 14, 2026
One was vestigial; three had a real access route already available or one line
away. Each was settled by compiling without the macro -- static inspection is
not reliable for this, as the discarded half of this batch shows (below).

- `read_sep_test.cpp` carries the macro but touches nothing private in
  `sep.h`. The directives are deleted and nothing else in the file 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. `Soc` already has a
  public `rotylm(i1, i2)` returning exactly `p_rot[l2plus1_*i1 + i2]`, so those
  become `soc.rotylm(i, n)` / `soc.rotylm(i+1, n)`. The one use left is
  `EXPECT_NE(soc.p_rot, nullptr)`, a guard that `rot_ylm()` allocated at all,
  which has no public equivalent; that gets `friend class SocTest;` and a
  one-line fixture helper.

- `atom_spec_test.cpp` called the private `Pseudopot_upf::read_pseudo_upf201`.
  `Pseudopot_upf` already grants `AtomPseudoTest`, `NCPPTest` and `ReadPPTest`
  from deepmodeling#7949; this adds `AtomSpecTest` beside them plus the usual 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
  straight into `Grid LatGrid(PARAM.input.test_grid)` -- PARAM used as a local,
  so it becomes one. Its genuine private access is `Grid::setMemberVariables`,
  which gets `friend class SltkGridTest;` and a wrapper. `Grid::pbc` and
  `Grid::sradius2`, also read by the test, 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.

`memory_test.cpp` and the three `propagator_test*.cpp` were in an earlier
revision of this change and are deliberately not here: the compiler showed both
assumptions wrong. `memory_test` reads `Memory::name`, `class_name`, `consume`
and `init_flag`, private statics reached through `::` rather than a member
access; the three propagator tests read `PARAM.input`, so they are reason-(a)
work, not vestigial macros. Both need their own treatment.

No test expectation changed. Macro occurrences 88 -> 84 across the tree; no
`#undef private` is added and no file whose macro survives is touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
… plus one friend (#7965)

`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 #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 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