Fix the harmful uses of #define private public - #7908
Open
Critsium-xy wants to merge 9 commits into
Open
Conversation
`cal_test.cpp` is compiled into the main library (source/source_io/CMakeLists.txt), not into a test target, and wrapped `parameter.h` in `#define private public`. That gave this translation unit a definition of `Parameter` in which `input` and `sys` are public, while every other translation unit in the library sees them as private -- one class with two different definitions in one program, i.e. an ODR violation. The hack was also unnecessary: the file only reads `PARAM.inp.nbands` and `PARAM.globalv.nlocal`, both public const references on `Parameter`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing includes this header -- it is referenced by no source file and by no CMakeLists.txt. It was also the only header in the tree that applied `#define private public`, and it applied it to 22 module headers at once (`unitcell.h`, `elecstate_lcao.h`, `hsolver_lcao.h`, `force_stress_lcao.h`, ...), so any translation unit that had included it would have compiled a large part of the codebase with access control disabled. On top of that it defined non-inline globals (`berryphase::berry_phase_flag`, `elecstate::Gatefield::zgate`, ...) at file scope, which would be a duplicate symbol as soon as a second translation unit included it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parameter.h` has always declared `friend class TestParameters` as the sanctioned way for unit tests to set parameter values, but the class itself was never defined anywhere shared: three test files each defined their own local `class TestParameters`, and 52 other test files reached for `#define private public` instead. Three local definitions of the same global-namespace class is itself an ODR hazard, so define it once in `source_io/module_parameter/test_parameters.h` and have the three existing users include it. Their purpose-built setters become file-local helpers in an anonymous namespace. The helper exposes mutable views of both the global `PARAM` and of a local `Parameter` object, since tests use both. No default arguments: the two forms are separate overloads. Header-only, so no CMakeLists change is needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
54 test translation units reached the private `Parameter::input` / `Parameter::sys`
members by wrapping `parameter.h` in `#define private public`. Replace all 1687
such accesses with the `TestParameters` accessors, so `Parameter` has one
definition everywhere.
Mechanical substitution:
PARAM.input.x -> TestParameters::input().x
PARAM.sys.x -> TestParameters::sys().x
param.input.x -> TestParameters::input(param).x (local Parameter)
param_in.input.x -> TestParameters::input(param_in).x
`parameter.h` is now included *before* the remaining `#define private public`
regions rather than inside them, so its include guard is already satisfied when
those regions are compiled and the class is never seen with access control
disabled -- this matters for the files that pulled it in transitively
(`read_input.h`, `setcell.h`, ...) rather than directly.
28 regions contained nothing but `parameter.h` and are removed outright.
`setcell.h` and `md_test_fixture.h` are test headers that were themselves
included inside a hacked region and are migrated the same way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six regions did nothing at all and are removed: - `unitcell_test_setupcell.cpp`, `unitcell_test_pw.cpp` had a bare `#define private public` immediately followed by `#undef private`, with no include in between -- left over from an earlier refactor. - `write_orb_info_test.cpp`, `rho_io_test.cpp`, `deltaspin_pw_test.cpp` and the second region of `bfgs_test.cpp` wrapped only `parameter.h`, and none of them touches a private member of `Parameter`. `test_diago_assist.cpp` is deleted. It is listed in no CMakeLists.txt, includes a header that does not exist (`diago_iter_assis.h`; the real name is `diago_iter_assist.h`), and has a statement in a class body (`DIAGOTEST::hamilt.create(4, 4);`), so it cannot compile. It has been dead since it was added in deepmodeling#6305. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These files opened `#define private public` (and often `#define protected public`) and never closed it, so from the include block onwards the whole translation unit compiled with `private` and `protected` meaning `public` -- up to 1016 lines in `charge_mixing_test.cpp`. `elecstate_pw_test.cpp` was a partial case: it closed `protected` 318 lines later, at the end of the file, and never closed `private`. Each `#undef` now sits immediately after the include block it is meant to cover. No include is moved, because in every one of these files the standard and system headers already came after the project headers -- they simply fall outside the region once it is closed. That removes the incidental treatment of `<omp.h>` (charge_mixing_test), `<string>` (elecstate_energy_test), `<fstream>` (verlet_test), `<mpi.h>` and `<source_base/module_external/scalapack_connector.h>` (propagator_test1/2/3), `<source_base/macros.h>` (test_hsolver) and `"mpi.h"` (read_wf2rho_pw_test). Closing a region early is safe: the class definitions the tests need to reach into have already been compiled by the time the `#undef` is seen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`source/source_base/test/CMakeLists.txt:1` already calls
`abacus_disable_feature_definitions(__MPI)`, so `__MPI` is never defined for any
target in that directory. Every `#ifdef __MPI` in these files is therefore always
false, which means the test bodies wrapped in one were compiled out in *every*
build configuration:
opt_cg_test.cpp Stand_Solve_LinearEq, PR_Solve_LinearEq,
HZ_Solve_LinearEq, PR_Min_Func, HZ_Min_Func
opt_tn_test.cpp TN_Solve_LinearEq, TN_Min_Func
math_chebyshev_test.cpp checkconverge, checkconverge_float
Nine tests that reported PASS while asserting nothing. The guards are removed so
the bodies compile and actually run.
The `#undef __MPI` / `#define __MPI` pairs inside those bodies were no-ops
regardless: the preprocessor cannot retroactively change headers that were already
processed above, and none of the bodies contains an `#ifdef __MPI` of its own. The
same goes for the three blocks in `global_function_test.cpp` (whose bodies sat
*outside* the `#ifdef` and so always compiled) and for the file-level toggles in
`opt_cg_test.cpp` / `opt_test_tools.cpp`, which duplicate what CMake already does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the dead `#ifdef __MPI` guards gone, the nine previously-compiled-out test
bodies run for the first time -- and abort in an MPI build:
*** The MPI_Allreduce() function was called before MPI_INIT was invoked.
That is why they were disabled. The test sources are compiled without `__MPI`
(the directory calls `abacus_disable_feature_definitions(__MPI)`), but they link
`base`, which is built *with* `__MPI`, so `Opt_CG` reaches
`Parallel_Reduce::reduce_all` (opt_cg.cpp:134 and 10 more). The old `#undef __MPI`
could never have prevented that: the library is a separate translation unit,
already compiled.
`opt_cg_test.cpp` / `opt_tn_test.cpp` (7 tests) are fixed with the mechanism the
build system already provides for this case (`cmake/Testing.cmake:38-44`, used the
same way in `source_estate/test/CMakeLists.txt:132`): the targets keep `__MPI`, and
each file gains a `main()` calling `MPI_Init`/`MPI_Finalize` under `#ifdef __MPI`,
matching `math_sphbes_test.cpp` in the same directory. All 7 now pass.
`math_chebyshev_test.cpp`'s `checkconverge` and `checkconverge_float` are handled
differently, because they need `POOL_WORLD` (via `GlobalFunc::ddot_real` ->
`Parallel_Reduce::reduce_pool`), not just `MPI_COMM_WORLD`. They are byte-identical
duplicates of the tests in `test_parallel/math_cheby_mpi_test.cpp`
(`MODULE_BASE_math_chebyshev_mpi`), which does set `POOL_WORLD` up correctly and
runs them. So they are deleted here rather than resurrected -- no coverage is lost.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The script that rewrote `PARAM.input` -> `TestParameters::input()` also rewrote 28 occurrences inside comments. Comments should keep naming the real field, so they are restored. That leaves `orb_atomic_lm_test.cpp` and `orb_nonlocal_lm_test.cpp` with no change at all: their only `PARAM.input` / `PARAM.sys` mentions were in comments, and `ninja -t deps` confirms neither translation unit reaches `parameter.h` even transitively, so they never needed the helper. Both are reverted to their original content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Critsium-xy
force-pushed
the
fix/test-access-control-hack
branch
from
September 4, 2026 07:09
032bfe6 to
8de0ae1
Compare
mohanchen
reviewed
Sep 4, 2026
Collaborator
There was a problem hiding this comment.
I don't like this solution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
#define private publicaround an#includeappears in 85 files in this tree(116 occurrences). It is not a harmless test trick: it gives that translation unit
a different definition of the class than the rest of the library it links
against, which is an ODR violation, and it silently applies to every other header
pulled in by the same region.
This PR does not try to remove the pattern everywhere. It fixes the cases where it
was actually doing damage, and gives the most common case a proper mechanism.
What is fixed
1. The one occurrence in production code
source_io/module_output/cal_test.cppis compiled into the main library(
source/source_io/CMakeLists.txt), not into a test target, and wrappedparameter.h. The shipped binary therefore contained one translation unit that sawParameter::input/Parameter::sysas public while every other one saw them asprivate.
The hack was also unnecessary: the file only reads
PARAM.inp.nbandsandPARAM.globalv.nlocal, both public.2.
parameter.his no longer compiled with access control disabled anywhereIt was, in 53 translation units — about a fifth of all wrapped includes.
parameter.hhas always declaredfriend class TestParametersas the sanctionedway for tests to set parameter values, but that class was never defined anywhere
shared: three test files each defined their own local copy (itself an ODR hazard),
and everyone else reached for the macro instead. This PR defines it once in
source_io/module_parameter/test_parameters.hand routes all 1686 test accessesthrough it:
parameter.his now included before the remaining regions rather than insidethem, so its include guard is already satisfied when a region is compiled. That is
what fixes the files which pulled it in transitively via
read_input.horsetcell.hrather than directly.After this, the only code in the tree touching
Parameter::input/Parameter::sysis its three declared friends:
ModuleIO::ReadInput,elecstate::ParamUpdater,and
TestParameters.3. Seventeen regions that were never closed
Sixteen of these are fixed in place (the seventeenth is the dead file deleted
below). They opened
#define private public(usuallyprotectedtoo) and ran toend of file — up to 1016 lines in
charge_mixing_test.cpp.elecstate_pw_test.cppclosedprotected318 lines later at the end of the file and never closedprivateatall.
Each
#undefnow sits immediately after the include block it is meant to cover. Noinclude is moved: in every one of these files the standard headers already came
after the project headers, so they simply fall outside the region once it is
closed. That removes the incidental treatment of
<omp.h>,<string>,<fstream>,<mpi.h>,<source_base/macros.h>,<source_base/module_external/scalapack_connector.h>and"mpi.h".4. Nine unit tests that reported PASS while asserting nothing
source/source_base/test/CMakeLists.txt:1already callsabacus_disable_feature_definitions(__MPI), so__MPIis never defined for thatdirectory and every
#ifdef __MPIin it is always false. Nine test bodies werewrapped in one, so they were compiled out in every configuration:
opt_cg_test.cpp(5),opt_tn_test.cpp(2),math_chebyshev_test.cpp(2).Removing the guards exposed why they had been disabled — in an MPI build they
abort with:
The test sources are compiled without
__MPI, but they linkbase, which is builtwith it, so
Opt_CGreachesParallel_Reduce::reduce_all(opt_cg.cpp:134 and 10more). The old
#undef __MPIcould never have prevented that — the library is aseparate translation unit, already compiled.
provides for this (
cmake/Testing.cmake:38-44, used the same way insource_estate/test/CMakeLists.txt:132): the targets keep__MPI, and each filegains a
main()callingMPI_Init/MPI_Finalize, matchingmath_sphbes_test.cppin the same directory. All 7 now run and pass withtheir original expected values.
math_chebyshev.checkconverge/checkconverge_floatneedPOOL_WORLD(viaGlobalFunc::ddot_real->Parallel_Reduce::reduce_pool), not justMPI_COMM_WORLD. They are byte-identical duplicates of the tests intest_parallel/math_cheby_mpi_test.cpp(MODULE_BASE_math_chebyshev_mpi), whichsets
POOL_WORLDup correctly and runs them. They are deleted here rather thanresurrected — no coverage is lost.
5. Dead code removed
source_io/test/for_testing_input_conv.h— included by nothing. It was the onlyheader applying
#define private public, and applied it to 22 module headers atonce; it also defined non-inline globals at file scope, which would be a
duplicate symbol the moment a second TU included it.
source_hsolver/test/test_diago_assist.cpp— in noCMakeLists.txt, includes aheader that does not exist (
diago_iter_assis.h), and has a statement in a classbody, so it cannot compile. Dead since [Refactor] Rename module_hsolver to source_hsolver #6305.
unitcell_test_setupcell.cppandunitcell_test_pw.cpphad abare
#define private publicimmediately followed by#undef private;write_orb_info_test.cpp,rho_io_test.cpp,deltaspin_pw_test.cppand thesecond region of
bfgs_test.cppwrapped onlyparameter.hand never touch aprivate member of
Parameter.Result
#define private/protected publicparameter.hwith access control offWhat is deliberately left
The remaining 59 files wrap the header of the class under test (
unitcell.h,klist.h,charge.h,sltk_grid.h, ...) to reach its private members. Thoseregions are now all balanced, tight, and free of standard headers, but the pattern
is still there. Removing it means giving each of those classes a test seam — a much
larger and more opinionated change, better done per module than in one PR.
read_wf2rho_pw_test.cppstill has a file-level#undef __LCAObefore itsincludes. That one is load-bearing (it selects the non-LCAO path in the headers
below it) and belongs in the test's CMakeLists as
abacus_disable_feature_definitions(__LCAO), but changing it changes what the testcompiles, so it is left for a separate change.
Review notes
PARAM.input->TestParameters::input()substitution (1686 sites, applied by script, uniform). The interesting commits
are the 1st, 3rd, 5th, 6th, 7th and 8th.
test_parameters.his header-only, so noCMakeLists.txtchange is needed forit.
PARAMand local-Parameterforms areseparate overloads.
agent_governance_check.py --base upstream/develop: no blockers. Theglobal-dependency budget moves by -921 (10 added, 931 removed). Remaining
warnings are the three added header includes (
test_parameters.hitself needsparameter.h;setcell.handmd_test_fixture.hneed the new helper) and "nodocs change", which is expected: no INPUT parameter, interface or runtime
behaviour changes outside test code.
Testing
Verified with gcc / Ninja,
BUILD_TESTING=ON, in two configurations:-DENABLE_LCAO=ON -DENABLE_MPI=OFF -DENABLE_OPENMP=ON-DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ONUnit tests, compared against an
upstream/developbuild in the identicalconfiguration (
ctest -E "^[0-9][0-9]_", serial,OMP_NUM_THREADS=1):upstream/developNo regressions and no differences at all -- the failing set is byte-identical
on both trees. Those 31 are pre-existing in this environment and are mostly
mpirun-based wrappers or in files this PR never touches (blas_connector,math_sphbes,cubic_spline,PSI_init,dav,bpcg, the*_para/*_paralleltargets).(Run under
-j4the count wobbles between 31 and 33 on both trees, becauseMODULE_BASE_memoryandMODULE_BASE_tool_quitare load-flaky; both pass 5/5standalone on both trees and neither is touched by this PR. The serial numbers
above are the flake-free comparison.)
The re-enabled tests specifically:
They pass against the expected values already written in those files — the
assertions had simply never been compiled.
Integration tests (
tests/) were not run here; they need the full toolchain build.