Skip to content

tests: take source_base and source_hamilt off #define private public, via accessors that already existed - #7982

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/quiet-corner-access-hacks
Sep 17, 2026
Merged

mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/quiet-corner-access-hacks

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

What this does

Takes source_base and source_hamilt to zero #define private public. In
both cases the class already had most of the interface the test needed; nothing
is opened up wholesale and no friend is declared.

source_hamilt/module_vdwvdw_test

Vdwd2Parameters already exposes C6(), R0(), damping() and scaling(),
and vdwd2.h's own index_loops() uses exactly those — while the test reached
past them into C6_ and R0_. Those reads now go through the accessors, and
radius() is added alongside the existing four. C6_input() / R0_input() were
public all along, so those four sites needed no change at all.

The one write, R0_["Si"] = 0.0 in D2R0ZeroQuit, goes through the public
R0_input() instead, reading a new one-line r0_zero.txt installed next to the
existing c6.txt / r0.txt. R0_input() does not validate against zero, so the
"R0_sum can not be 0" guard in index_loops() is still exactly what the test
hits.

source_basememory_test

The finish test fabricated a record entry by writing name, class_name,
consume and init_flag directly. That setup was also unsound: setting
init_flag = true made the record() call two lines later skip its own
allocation, so *name = ... wrote through whatever a previous test had left
behind — and through a null pointer in any execution order where record() had
not run yet.

It now just calls record(), which allocates the tables and adds the entry
finish() is meant to print and release, and reads the result back through a new
is_initialized(), added next to the get_total() that was already public for
the same purpose.

Assertions and expected values are unchanged in both files. No default
arguments were added. No PARAM/GlobalV/GlobalC reference is added or
removed. No INPUT parameter or user-facing behaviour changed, so no documentation
update is required.

Scope: what I took out, and why

This PR started as a four-file batch. Two files were dropped after measurement,
and I would rather say so than force them through:

  • source_pw/module_pwdft/structure_factor_test — the twelve reads of
    c_eigts* / z_eigts* substitute cleanly into the already-public
    get_eigts1_data<FPTYPE>() / 2 / 3, with no production change. But the
    test also writes PARAM.sys.has_float_data, which Structure_Factor::setup()
    reads, so the macro only dies if that becomes a parameter — and that turned out
    to be wider than it looks:

    • Structure_Factor::setup has 8 call sites, not the 3 a first grep
      suggested. Two of them are mock definitions in other modules'
      tests (source_psi/test/psi_init_unit_test.cpp,
      source_estate/test/charge_extra_test.cpp), plus
      source_hamilt/module_surchem/test/cal_pseudo_test.cpp.
    • charge_extra.cpp currently has no PARAM reference at all; passing the
      flag from there would mean adding parameter.h and two global reads to a
      file that is clean today — the opposite of the direction this campaign is
      going.

    So that one belongs in its own PR, where the has_float_data plumbing can be
    routed properly rather than pushed into a clean file.

  • source_estate/elecstate_occupy_test — needs both nspin injected into
    occupy.cpp (2 reads) and the four static private helpers (efermig,
    sumkg, wgauss, w1gauss, 13 test sites, ~6 internal callers) given a real
    interface. All four take their inputs as parameters and touch no private class
    state, so extracting them into a namespace is the right fix — but it is a
    bigger, self-contained job.

Verification

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

  • configure exit 0; build exit 0, zero errors; ninja: no work to do
    on a follow-up full build
  • MODULE_BASE_memory and MODULE_HAMILT_vdwTest both link
  • agent_governance_check.py --base upstream/develop --head HEAD: zero
    blockers
    , no access-hack ratchet finding (2 removed, 0 added)

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

🤖 Generated with Claude Code

Two modules go to zero access hacks. In both cases the class already had most of
the interface the test needed; nothing is opened up wholesale and no friend is
declared.

source_hamilt/module_vdw (vdw_test)
  Vdwd2Parameters already exposes C6(), R0(), damping() and scaling() -- and
  vdwd2.h's own index_loops() uses exactly those -- while the test reached past
  them into C6_ and R0_. Those reads now go through the accessors, and radius()
  is added alongside the existing four. C6_input()/R0_input() were public all
  along.
  The one write, R0_["Si"] = 0.0 in D2R0ZeroQuit, goes through the public
  R0_input() instead, reading a new one-line r0_zero.txt installed next to the
  existing c6.txt / r0.txt. R0_input() does not validate against zero, so the
  "R0_sum can not be 0" guard in index_loops() is still what the test hits.

source_base (memory_test)
  The finish test fabricated a record entry by writing name, class_name,
  consume and init_flag. Setting init_flag = true made the record() call two
  lines later skip its own allocation, so *name = ... wrote through whatever a
  previous test had left behind -- and through a null pointer in any order where
  record() had not run yet. It now just calls record(), which allocates the
  tables and adds the entry finish() is meant to print and release, and reads
  the result back through a new is_initialized(), added next to the get_total()
  that was already public for the same purpose.

Assertions and expected values are unchanged in both files.

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

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