Skip to content

source_cell: give stateless private helpers a real interface instead of friendship - #7964

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/pure-helpers-to-free-functions
Sep 14, 2026
Merged

mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/pure-helpers-to-free-functions

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

source_cell: give stateless private helpers a real interface instead of friendship

The #define private public cleanup has been reaching for
friend class XxxTest; whenever a test calls a private method. For part of
those methods that is the wrong tool: measuring them shows they never touch
this at all. A function that uses only its arguments is not a member function,
and a test calling it needs neither friendship nor a wrapper -- it needs the
function to be declared where it can be called.

Of the ten private methods this series has granted friendship for, six use no
object state. This change deals with all six, and two of them turn out not to
need to exist.

Two are dead code kept reachable only by a test

Magnetism::judge_parallel has no caller anywhere and is a character-for-
character duplicate of the free function unitcell::judge_parallel in
cal_ux.h, which is used (cal_ux.cpp) and is already covered by
UcellTest.JudgeParallel. The duplicate, its friend class MagnetismTest; and
the fixture wrapper are removed.

Coverage improves rather than shrinks: MagnetismTest.JudgeParallel checked
both a parallel and a non-parallel pair, while UcellTest.JudgeParallel only
checked the parallel one. The negative case moves to UcellTest, so the
surviving, actually-used function is now better tested than before.

Pseudopot_upf::trimend likewise has no production caller -- only its own
definition and a test reaching through the access hack. Removed, along with the
half of ReadPPTest.Trim that exercised it.

Four become plain functions

set_pseudo_type, trim, setqfnew and complete_default_h move out of
Pseudopot_upf into namespace pseudopot, declared in read_pp.h. Their
bodies are unchanged; the internal callers (init_pseudo_reader,
complete_default, the qfunc expansion in read_pp.cpp, and getnameval in
read_pp_upf201.cpp) gain the qualifier. Six forwarding wrappers disappear from
read_pp_test.cpp and pseudo_nc_test.cpp, which now call the functions
directly.

This is better than the alternatives it replaces: unlike friend, it grants no
special access to anyone; unlike making them public members, it does not widen
the interface of Pseudopot_upf at all.

What this does not achieve

No Pseudopot_upf friend declaration is removed. All four fixtures still need
read_pseudo_upf201, which writes thirteen members and is genuinely stateful.
The only friendship eliminated here is Magnetism's, and only because its sole
private member was the dead duplicate.

The remaining four friended methods -- read_pseudo_upf201 (13 this->),
Numerical_Orbital_Lm::cal_kradial (18) and plot (26), and
Grid::setMemberVariables (34) -- cannot be handled this way: as free functions
they would take either the whole object, which gains nothing, or a dozen
parameters, which is worse. Extracting a pure computational core from them is a
production refactor and belongs in its own proposal, not in a test-access
cleanup.

Verification

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

  • Build: the only failing target is
    source_lcao/module_lr/.../lr_io_krlist.cpp.o, on
    ri_util.h: RI/global/Array_Operator.h: No such file or directory. That is
    pre-existing on develop in an environment without LibRI headers (from
    Refactor LR_IO function and add test case 58_KP_LR_BSE #7849); this PR touches neither file. Every other target compiles, and the
    compiler reported no member-access error anywhere, which is the direct
    confirmation that these six functions really were stateless.
  • All twelve affected tests pass: MODULE_CELL_read_pp,
    MODULE_CELL_pseudo_nc, MODULE_CELL_atom_pseudo, MODULE_CELL_atom_spec,
    MODULE_CELL_magnetism and the seven MODULE_CELL_unitcell_test* variants.
  • agent_governance_check.py --base upstream/develop --head HEAD: 0 errors.

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

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

🤖 Generated with Claude Code

…of friendship

The `#define private public` cleanup has been reaching for
`friend class XxxTest;` whenever a test calls a private method. For part of
those methods that is the wrong tool: measuring them shows they never touch
`this` at all. A function that uses only its arguments is not a member function,
and a test calling it needs neither friendship nor a wrapper -- it needs the
function to be declared where it can be called.

Of the ten private methods this series has granted friendship for, six use no
object state. This change deals with all six, and two of them turn out not to
need to exist.

## Two are dead code kept reachable only by a test

`Magnetism::judge_parallel` has **no caller anywhere** and is a character-for-
character duplicate of the free function `unitcell::judge_parallel` in
`cal_ux.h`, which *is* used (`cal_ux.cpp`) and *is* already covered by
`UcellTest.JudgeParallel`. The duplicate, its `friend class MagnetismTest;` and
the fixture wrapper are removed.

Coverage improves rather than shrinks: `MagnetismTest.JudgeParallel` checked
both a parallel and a non-parallel pair, while `UcellTest.JudgeParallel` only
checked the parallel one. The negative case moves to `UcellTest`, so the
surviving, actually-used function is now better tested than before.

`Pseudopot_upf::trimend` likewise has no production caller -- only its own
definition and a test reaching through the access hack. Removed, along with the
half of `ReadPPTest.Trim` that exercised it.

## Four become plain functions

`set_pseudo_type`, `trim`, `setqfnew` and `complete_default_h` move out of
`Pseudopot_upf` into `namespace pseudopot`, declared in `read_pp.h`. Their
bodies are unchanged; the internal callers (`init_pseudo_reader`,
`complete_default`, the qfunc expansion in `read_pp.cpp`, and `getnameval` in
`read_pp_upf201.cpp`) gain the qualifier. Six forwarding wrappers disappear from
`read_pp_test.cpp` and `pseudo_nc_test.cpp`, which now call the functions
directly.

This is better than the alternatives it replaces: unlike `friend`, it grants no
special access to anyone; unlike making them public members, it does not widen
the interface of `Pseudopot_upf` at all.

## What this does not achieve

No `Pseudopot_upf` friend declaration is removed. All four fixtures still need
`read_pseudo_upf201`, which writes thirteen members and is genuinely stateful.
The only friendship eliminated here is `Magnetism`'s, and only because its sole
private member was the dead duplicate.

The remaining four friended methods -- `read_pseudo_upf201` (13 `this->`),
`Numerical_Orbital_Lm::cal_kradial` (18) and `plot` (26), and
`Grid::setMemberVariables` (34) -- cannot be handled this way: as free functions
they would take either the whole object, which gains nothing, or a dozen
parameters, which is worse. Extracting a pure computational core from them is a
production refactor and belongs in its own proposal, not in a test-access
cleanup.

## Verification

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

- Build: the only failing target is
  `source_lcao/module_lr/.../lr_io_krlist.cpp.o`, on
  `ri_util.h: RI/global/Array_Operator.h: No such file or directory`. That is
  pre-existing on `develop` in an environment without LibRI headers (from
  deepmodeling#7849); this PR touches neither file. Every other target compiles, and the
  compiler reported no member-access error anywhere, which is the direct
  confirmation that these six functions really were stateless.
- All twelve affected tests pass: `MODULE_CELL_read_pp`,
  `MODULE_CELL_pseudo_nc`, `MODULE_CELL_atom_pseudo`, `MODULE_CELL_atom_spec`,
  `MODULE_CELL_magnetism` and the seven `MODULE_CELL_unitcell_test*` variants.
- `agent_governance_check.py --base upstream/develop --head HEAD`: 0 errors.

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Sep 14, 2026
@mohanchen
mohanchen merged commit dcb849e into deepmodeling:develop Sep 14, 2026
17 checks passed
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