Skip to content

tests: remove fifteen #define private public — by deleting dead hacks and by passing INPUT values explicitly - #7980

Merged
mohanchen merged 2 commits into
deepmodeling:developfrom
Critsium-xy:refactor/drop-vestigial-access-hacks
Sep 17, 2026
Merged

mohanchen merged 2 commits into
deepmodeling:developfrom
Critsium-xy:refactor/drop-vestigial-access-hacks

Conversation

@Critsium-xy

@Critsium-xy Critsium-xy commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

What this does

Removes 15 of the 69 #define private public / #define protected public
macros in source/ (69 → 54), in two commits. No friend declaration is
added anywhere
— every macro is removed either because it was never needed, or
by making the dependency explicit, which is why the diff reduces global-state
coupling instead of relocating it.

PARAM/GlobalV/GlobalC ledger for the whole PR: added 5, removed 31,
net_delta = −26
.


Commit 1 — five macros no test actually needed

Four were vestigial: the tests inside them access no private or protected member
of any class in the headers the macro covers.

File Why the macro could go
source_cell/test/klist_test_para.cpp The only mentions of K_Vectors::spin_mult and mpi_k() are in comments. koffset at lines 209/333 is a local const double koffset[3], not the private member of the same name.
source_cell/test/qlist_test.cpp Every member it reads — nkstot, nkstot_nospin, wk, kvec_c, kvec_d, kc_done, kd_done, is_mp — is public in ModuleCell::ReciprocalGrid. QList's own privates (nirr_, irrep_modes_, little_group_, interpolate_q_between, get_irreps) are never touched.
source_cell/test/sepcell_test.cpp Already goes through the public getters get_ntype(), get_omega(), get_tpiba2(), get_sep_enable(), get_seps(). sep_enable appears only inside a comment.
source_lcao/test/test_output_hcontainer_consistency.cpp Uses none of the private members of HContainer, Output_HContainer or Read_HContainer.
source_lcao/test/test_init_dm_from_file.cpp Read DensityMatrix::_DMR directly in 7 places while calling the public get_DMR_vector() in the same tests. get_DMR_vector() returns exactly _DMR (density_matrix.h:191), so those 7 reads now go through it.

The three source_cell macros also covered pseudo.h, atom_spec.h,
magnetism.h and atom_pseudo.h, which have no private sections at all
including them under the macro never did anything.

Commit 2 — ten macros that existed only to write the private half of PARAM

Group A: the dependency was already injected; the test routed through the global for no reason

No production change at all in this group.

  • propagator_test{1,2,3} (6 macros) — Propagator's constructor already
    takes const double& dt. Each test wrote PARAM.input.mdp.md_dt and then read
    PARAM.mdp.md_dt straight back to pass it in. Now a local md_dt. The
    now-unused parameter.h include is dropped; propagator.h already provides
    ModuleBase::AU_to_FS via source_base/constants.h, and
    source_base/global_variable.h is included directly for the
    GlobalV::ofs_running these tests genuinely use.
  • single_r_io_test (1) — PARAM.sys.nlocal = 99 was dead.
    single_r_io.cpp has no PARAM reference at all and takes nlocal from
    pv.get_global_row_size(), which this test stubs to return 5.
  • read_wfc_nao_test (1) — read_wfc_nao() already takes the directory as
    its first argument; the test wrote PARAM.sys.global_readin_dir and passed
    it straight back in. Now a local readin_dir.

Group B: the production code really did read PARAM, so the value became a parameter

  • write_dmk() now takes const std::string& dmk_dir, mirroring its sibling
    read_dmk() in the same header, which already does. write_dmk.cpp is now
    PARAM-free and drops the include. Its one caller
    (ctrl_scf_lcao.cpp:199) already holds a global_out_dir local, so the call
    site adds no PARAM reference.
  • K_Vectors::set_spin_mult() added next to the existing set_nks() /
    set_nkstot() / set_nkstot_nospin(). The public getter get_spin_mult()
    already existed, so this completes an incomplete setter group and lets
    write_dmk_test stop assigning kv.spin_mult directly — the second reason
    that file needed the macro.
  • write_eig_iter() and write_eig_file() take nbands and nspin, and
    write_eig_file() takes const std::string& out_dir. Both callers are in
    esolver_ks.cpp, which already holds an injected inp_, so nbands/nspin
    cost no PARAM reference.
  • write_eig_occ_test's PARAM.sys.nbands_l write was dead (no source or
    library in that target reads it — the readers are in source_estate,
    source_psi and source_pw), and its PARAM.input.bndpar read is now a
    local mirroring the Input_para default of 1, so the test no longer depends
    on an INPUT default.

No default arguments were added (governance blocks that, and print_force
already showed why: adding one is not an option). No assertion or expected
value changed.

Governance warnings, with rationale

  • Global dependency budget — 5 added vs 31 removed, net_delta = −26.
    • esolver_ks.cpp:328 (+1) is the only genuinely new reference:
      PARAM.globalv.global_out_dir at the write_eig_file call site. This is the
      composition root deliberately absorbing a global read so the leaf writer stays
      clean — the same direction as source_relax: pass INPUT values explicitly instead of reading PARAM, removing the test access hacks it caused #7921.
    • write_eig_occ_test.cpp:66,67 (+4) are pre-existing GlobalV::NPROC /
      GlobalV::MY_RANK arguments to Parallel_Global::init_pools; reflowing that
      call to pass a local bndpar makes the checker count those lines as added.
  • Header dependencywrite_eig_occ.h gains <string>, required by the new
    const std::string& out_dir parameter. No forward declaration is possible for
    std::string here.
  • Documentation — no INPUT parameter or user-facing behaviour changed, so no
    docs/ update is required and docs/parameters.yaml needs no regeneration.

Not in this PR

  • source_io/module_json/test/para_json_test.cpp — reads the private static
    Json::AbacusJson::doc 89 times (77 as doc[...] subscripts). Its only
    public path is write_to_json(filename). Also note this target builds only
    under ENABLE_RAPIDJSON.
  • source_estate/module_dm/test/test_dm_r_init.cpp — asserts on
    DensityMatrix::_DMR_save[0].size() in 2 places; needs a production accessor.
  • source_io/test/outputlog_test.cppout_level and nspin are cheap
    (1 call site each), but its third key test_force lives in print_force,
    which has ~45 call sites across force_pw.cpp, sto_forces.cpp,
    force_stress_assemble.cpp, md_func.cpp and esolver_{dp,lj,nep}.cpp.
    Injecting it would add ~45 PARAM references and block on the budget, all
    for one std::cout << table; debug echo. It needs its own PR paired with a
    real reduction elsewhere.

Continues #7921 / #7949 / #7952 / #7953 / #7963 / #7964 / #7965 / #7966 / #7967
and keeps their scope rule: a file is either taken fully off the macro or left
alone.

Verification

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

  • configure exit 0; 3354/3354 targets, build exit 0, zero errors,
    ninja: no work to do on a follow-up full build
  • every changed file confirmed compiled, and all affected targets link:
    MODULE_CELL_klist_test_para1, MODULE_CELL_qlist_test,
    MODULE_CELL_SEP_CELL_TEST, MODULE_LCAO_init_dm_from_file_test,
    MODULE_LCAO_output_hcontainer_consistency_test,
    MODULE_LCAO_tddft_propagator_test, MODULE_IO_single_R_test,
    MODULE_IO_read_wfc_nao_test, MODULE_IO_write_dmk,
    MODULE_IO_write_eig_occ_test, plus abacus_basic_para
  • agent_governance_check.py --base upstream/develop --head HEAD: zero
    blockers
    , no access-hack ratchet finding (15 removed, 0 added)

🤖 Generated with Claude Code

Critsium-xy and others added 2 commits September 17, 2026 11:20
Four of these macros were vestigial: the tests inside them touch no private
or protected member of any class in the headers they cover.

  - klist_test_para.cpp   the only mentions of K_Vectors::spin_mult and
                          mpi_k() are in comments; koffset is a local array,
                          not the private member of the same name
  - qlist_test.cpp        every member it reads (nkstot, nkstot_nospin, wk,
                          kvec_c, kvec_d, kc_done, kd_done, is_mp) is public
                          in ModuleCell::ReciprocalGrid; QList's own privates
                          (nirr_, irrep_modes_, little_group_) are untouched
  - sepcell_test.cpp      already goes through the public getters get_ntype(),
                          get_omega(), get_tpiba2(), get_sep_enable() and
                          get_seps(); sep_enable appears only in comments
  - test_output_hcontainer_consistency.cpp
                          uses none of HContainer's, Output_HContainer's or
                          Read_HContainer's private members

The fifth, test_init_dm_from_file.cpp, read DensityMatrix::_DMR directly in
seven places while already calling the public get_DMR_vector() three lines
away in the same tests. get_DMR_vector() returns exactly _DMR, so those seven
reads are now routed through it.

No production code changes and no assertion changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…removing ten access hacks

Ten `#define private public` / `#define protected public` existed only so a
test could write the private half of PARAM. None of them is replaced by a
friend declaration: each is removed by making the dependency explicit, so the
diff removes 31 PARAM/GlobalV occurrences and adds 5 (net_delta = -26).

Group 1 -- the dependency was already injected; the test was routing through
the global for no reason. No production change at all:

  - propagator_test{1,2,3}: Propagator's constructor already takes
    `const double& dt`. Each test wrote PARAM.input.mdp.md_dt and then read
    PARAM.mdp.md_dt straight back to pass it in. Now a local `md_dt`, and the
    (henceforth unused) parameter.h include is dropped -- propagator.h already
    provides ModuleBase::AU_to_FS via source_base/constants.h.
  - single_r_io_test: `PARAM.sys.nlocal = 99` was dead. single_r_io.cpp has no
    PARAM reference at all and takes nlocal from pv.get_global_row_size(),
    which this test stubs to return 5.
  - read_wfc_nao_test: read_wfc_nao() already takes the directory as its first
    argument; the test wrote PARAM.sys.global_readin_dir and passed it back in.
    Now a local `readin_dir`.

Group 2 -- the production code really did read PARAM, so the value is now a
parameter:

  - write_dmk() takes `const std::string& dmk_dir`, mirroring its sibling
    read_dmk() which already does. write_dmk.cpp is now PARAM-free and drops
    the include. Its one caller (ctrl_scf_lcao.cpp) already holds a
    `global_out_dir` local, so the call site adds no PARAM reference.
  - K_Vectors gains `set_spin_mult()` next to the existing set_nks() /
    set_nkstot() / set_nkstot_nospin(); the public getter get_spin_mult()
    already existed, so this completes an incomplete setter group and lets
    write_dmk_test stop assigning kv.spin_mult directly.
  - write_eig_iter() and write_eig_file() take nbands and nspin, and
    write_eig_file() takes `const std::string& out_dir`. Both callers are in
    esolver_ks.cpp, which already holds an injected `inp_`, so nbands/nspin
    cost no PARAM reference; only out_dir adds one.
  - write_eig_occ_test's PARAM.sys.nbands_l write was dead (no source or
    library in that target reads it) and its PARAM.input.bndpar read is now a
    local mirroring the Input_para default of 1, so the test no longer depends
    on an INPUT default.

No default arguments were added. No assertion or expected value changed.
write_eig_occ.cpp still reads PARAM for out_alllog, calculation and
mdp.md_restart -- no test drives those, so injecting them would only cost
budget.

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 17, 2026
@mohanchen
mohanchen merged commit 98b141a into deepmodeling:develop Sep 17, 2026
33 checks passed
Zanthoxylum pushed a commit to Zanthoxylum/abacus-develop that referenced this pull request Sep 17, 2026
mohanchen added a commit that referenced this pull request Sep 18, 2026
…to zero access hacks (#7987)

klist_test reached into K_Vectors through `#define private public`. It turned out
that only four of the things it touches actually needed anything, and two needed
nothing at all.

Needed wrappers -- set() is the single production entry point and drives these in
order, while the test exercises them one stage at a time because most of the
KPT-file parsing paths are only reachable that way:

  read_kpoints_for_testing()        27 call sites
  renew_for_testing()                8
  reduce_by_symmetry_for_testing()   2
  set_kup_and_kdw_for_testing()      5

Needed nothing:

  - spin_mult, 37 sites (36 writes, 1 read), already has public get_spin_mult()
    and set_spin_mult(); the setter was added in #7980 for write_dmk_test and
    covers every one of them.
  - koffset, 4 sites. Both Monkhorst_Pack() and read_kpoints() take the offset as
    an argument, and the 27 read_kpoints call sites already pass a local
    `const double koffset[3]`. Only two setup blocks wrote the member and then
    handed it straight back to Monkhorst_Pack(), so those now use a local array
    too and the member is not touched from the test at all.

The remaining 155 accesses in the file (get_nkstot, kvec_c, kvec_d, wk, kc_done,
kd_done, set_both_kvec, nmp, isk, ...) were public throughout and are unchanged.
I checked all six K_Vectors objects in the file -- kv, kv1 and kv_test1..4 -- and
the other classes the macro covered: nothing else private is used, and pseudo.h,
atom_spec.h, atom_pseudo.h and magnetism.h have no private sections at all.

Worth stating for review: the four wrappers are public entry points that only the
tests use, in the style of set_density_rotations_for_testing() in
symm_rotation.h, and carry a comment saying production code must keep going
through set(). read_kpoints() in particular is a substantial piece of behaviour
with 27 tests behind it; if review would rather see it simply become part of the
public interface, that is a one-line change and the wrapper can go.

No production logic changed. No assertion or expected value changed.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Mohan Chen <mohanchen@pku.edu.cn>
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