Follow-ups from #281: unneeded Boost type traits in HDF5 archive; MEASURE_* parsing untested

Aperta
  • Qualcosa non va?
#290 0 commenti 0 reazioni 1 assegnatario Vedi su GitHub

I maintainer di solito rispondono entro 3 giorni

@Ooolab ci sta già lavorando.

Dal 3/11/2026.

Valutazione

Questa issue non è ancora stata valutata.

Descrizione

area:build-sdk area:ci-tests area:ngs area:scheduler area:tools

Follow-ups found while reviewing #281 (part of #275). None of them block #281.

1. archive.cpp needs two Boost headers for type traits that do nothing

#281 adds <boost/type_traits/remove_cv.hpp> and <boost/type_traits/remove_reference.hpp> to src/alps/hdf5/src/archive.cpp. Before #281 they came in through <boost/thread.hpp>. Without them the file doesn't compile.

Both traits are used only inside the ALPS_NGS_HDF5_WRITE_SCALAR macro (archive.cpp:1072 and archive.cpp:1106):

detail::native_ptr_converter<boost::remove_cv<boost::remove_reference<T>::type>::type> converter(1);

The macro defines archive::write(std::string path, T value) and is expanded only through ALPS_NGS_FOREACH_NATIVE_HDF5_TYPE (archive.hpp:43), which lists char through long double, bool and std::string. T is always one of these bare types and value is taken by value. remove_cv<remove_reference<T>> gives back T for every one of them.

Fix: use T directly and remove the two includes:

detail::native_ptr_converter<T> converter(1);

The native_ptr_converter<std::string> specialization still matches, because T is exactly std::string in that expansion.

2. No test runs the MEASURE_* parameter parsing

src/alps/scheduler/src/scheduler/measurement_operators.C reads MEASURE_AVERAGE[...], MEASURE_LOCAL[...], MEASURE_CORRELATIONS[...] and MEASURE_STRUCTURE_FACTOR[...] from the input parameters. #281 moved its regular expressions from boost::regex to std::regex. The class is used by fulldiag/sparsediag (alps/scheduler/diag.hpp), the QMC apps (src/apps/qmc/qmc.h) and the loop code (src/apps/qmc/looper/looper/custom.h).

No native or Python test runs an application with these parameters:

  • No ctest passes MEASURE_* keys.
  • python/pyalps/tests/fixtures/application_results/fulldiag.parm contains MEASURE_LOCAL[Local magnetization]=Sz, but test_legacy_results.py only loads the frozen .h5 files. It does not run fulldiag.

A side-by-side check outside the test suite gave the same matches and captures for std::regex and boost::regex on realistic keys: plain names, nested ], empty brackets, a:b:c. The results differ only when a key or value contains \n or \r. The text parameter grammar never produces such a key or value.

Fix: add a ctest that runs fulldiag on a 4-site chain with one key of each MEASURE_* kind (including a MEASURE_CORRELATIONS[...]=A:B pair) and checks that the observables appear in the output with the expected names and values.

3. Remove the unused alps/ngs/mutex.hpp and alps/ngs/thread_exceptions.hpp

#281 changed alps::native_lockable in src/alps/utilities/include/alps/ngs/mutex.hpp from boost::mutex to std::mutex. Nothing in ALPS uses the header:

  • No source file, test or tutorial includes alps/ngs/mutex.hpp. It appears only in the install list (src/alps/utilities/CMakeLists.txt:65) and in tests/cmake/preserved_public_headers.json.
  • Nothing uses native_lockable, noop_lockable or detail::mutex_derived.
  • alps/ngs/thread_exceptions.hpp is included only by mutex.hpp. It is the last <boost/thread/...> include in ALPS (<boost/thread/exceptions.hpp>).

The header belongs to the NGS layer whose parameters, Monte Carlo base, observables and accumulators are already retired. ALPS_NGS_SINGLE_THREAD is still read by the HDF5 archive lock and cli/mcoptions.cpp, and is not affected.

Fix: remove both headers and their entries in src/alps/utilities/CMakeLists.txt and tests/cmake/preserved_public_headers.json, and add a CHANGELOG line. Downstream code that includes them stops compiling, so as an alternative, mark them deprecated for one release as #210 did for the BLAS headers.

4. ALPS_NGS_SINGLE_THREAD is never defined, and --threaded does nothing

ALPS_NGS_SINGLE_THREAD has no definition: src/alps/utilities/include/alps/ngs/config.hpp:20 has it commented out (// #define ALPS_NGS_SINGLE_THREAD) and no CMake option sets it. As a result:

  • The #ifndef ALPS_NGS_SINGLE_THREAD code always compiles. This includes the HDF5 archive lock (archive.hpp:31,235, archive.cpp:33,1423), which is live and is the lock #281 moved to std::recursive_mutex.
  • The #ifdef ALPS_NGS_SINGLE_THREAD branches never compile: the empty ALPS_HDF5_LOCK_MUTEX in archive.cpp:33, the "Not build with multithread support" error in src/alps/cli/src/mcoptions.cpp:57, and the single-thread branches in thread_exceptions.hpp:17 and src/alps/cli/tests/cli_contract.cpp:77-87.
  • --threaded (mcoptions.cpp:32) is always offered and sets mcoptions::THREADED, or HYBRID together with --mpi. Outside mcoptions.cpp and cli_contract.cpp, nothing reads THREADED or HYBRID, so the option is accepted and has no effect.

Fix: make the #ifndef code unconditional, delete the #ifdef branches and the commented-out line in config.hpp, and update cli_contract.cpp. Either remove --threaded along with THREADED and HYBRID, or keep it and document that it has no effect. Removing it changes the command line, so it needs a CHANGELOG line. The cleanup overlaps item 3 if thread_exceptions.hpp is removed.

Lingua principale
C++
Stelle
32
Fork
10
Merge medio
4g 5h
PR unite (30g)
52

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di ALPSim/ALPS

Tutte le issue di ALPSim/ALPS

Issue simili

Altre issue su C++

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.