diff --git a/src/QMCDrivers/WalkerLogBuffer.h b/src/QMCDrivers/WalkerLogBuffer.h index e7ce2646f2..d73b1044e7 100644 --- a/src/QMCDrivers/WalkerLogBuffer.h +++ b/src/QMCDrivers/WalkerLogBuffer.h @@ -151,6 +151,9 @@ class WalkerLogBuffer public: WalkerLogBuffer(); + WalkerLogBuffer(const WalkerLogBuffer& other) = default; + WalkerLogBuffer(WalkerLogBuffer&& other) = default; + WalkerLogBuffer& operator=(WalkerLogBuffer&& other) = default; /// current number of rows in the data buffer inline size_t nrows() { return buffer.size(0); } diff --git a/src/QMCDrivers/WalkerLogCollector.h b/src/QMCDrivers/WalkerLogCollector.h index 39fd3a1185..eb89341e04 100644 --- a/src/QMCDrivers/WalkerLogCollector.h +++ b/src/QMCDrivers/WalkerLogCollector.h @@ -96,13 +96,22 @@ class WalkerLogCollector Array Gtmp; /// tmp storage for walker wavefunciton laplacians Array Ltmp; - /// state data set by WalkerLogManager - const WalkerLogState& state_; + /** Hopefully This is just a bundle of constructor arguments. + * It was a reference, so perhaps, the intention was dynamic + * manipulation of a group of objects state from afar. + * Since it hadn't yet been used this way its just a private member + * now. _reviewers_ do not allow it to be made a reference again. + * + * If you want to do a state transform write a function, document + * it, call it from a sensible and obvious scope. + */ + WalkerLogState state_; public: /// constructor. The state should be given by the manager. WalkerLogCollector(const WalkerLogState& state); - + WalkerLogCollector(WalkerLogCollector&& other) = default; + WalkerLogCollector& operator=(WalkerLogCollector&& other) = default; /// resize buffers to zero rows at beginning of each MC block void startBlock(); diff --git a/src/QMCDrivers/WalkerLogManager.cpp b/src/QMCDrivers/WalkerLogManager.cpp index 463f3c26ac..9dd736f7da 100644 --- a/src/QMCDrivers/WalkerLogManager.cpp +++ b/src/QMCDrivers/WalkerLogManager.cpp @@ -77,7 +77,6 @@ WalkerLogManager::WalkerLogManager(WalkerLogInput& inp, bool allow_logs, std::st registered_hdf = false; } - std::unique_ptr WalkerLogManager::makeCollector() const { if (state.verbose) diff --git a/src/QMCDrivers/WalkerLogManager.h b/src/QMCDrivers/WalkerLogManager.h index 67bf96c36a..0818a9d72d 100644 --- a/src/QMCDrivers/WalkerLogManager.h +++ b/src/QMCDrivers/WalkerLogManager.h @@ -31,14 +31,14 @@ struct WalkerLogInput; * to the HDF file at the end of each MC block. * * Just prior to the write, this class examines the distribution of walker - * energies on its rank for each MC step in the MC block, identifies the - * minimum/maximum/median energy walkers and buffers their full data + * energies on its rank for each MC step in the MC block, identifies the + * minimum/maximum/median energy walkers and buffers their full data * (including per-particle information) for the write. - * This data corresponds to walker information at specific quantiles of the + * This data corresponds to walker information at specific quantiles of the * energy distribution. * See WalkerLogManager::writeBuffers() * - * Writing per-particle information for all walkers is optional, as is + * Writing per-particle information for all walkers is optional, as is * writing data for the minimum/maximum/median energy walkers. * Scalar "property" data for each walker is always written. */ @@ -104,7 +104,8 @@ class WalkerLogManager public: WalkerLogManager(WalkerLogInput& inp, bool allow_logs, std::string series_root, Communicate* comm = 0); - + WalkerLogManager& operator=(WalkerLogManager&& other) = default; + WalkerLogManager(WalkerLogManager&& other) = default; /// create a WalkerLogCollector std::unique_ptr makeCollector() const; diff --git a/src/QMCDrivers/tests/CMakeLists.txt b/src/QMCDrivers/tests/CMakeLists.txt index 863f56c6aa..575947b13c 100644 --- a/src/QMCDrivers/tests/CMakeLists.txt +++ b/src/QMCDrivers/tests/CMakeLists.txt @@ -34,7 +34,8 @@ set(DRIVER_TEST_SRC test_vmc_driver.cpp test_dmc_driver.cpp test_SimpleFixedNodeBranch.cpp - test_QMCDriverFactory.cpp) + test_QMCDriverFactory.cpp + test_WalkerLogManager.cpp) # legacy driver mpi, not tested in a meaningful way since the standard unit test is # just one rank with mpi. diff --git a/src/QMCDrivers/tests/ValidWalkerLogInput.h b/src/QMCDrivers/tests/ValidWalkerLogInput.h new file mode 100644 index 0000000000..e8cdb54381 --- /dev/null +++ b/src/QMCDrivers/tests/ValidWalkerLogInput.h @@ -0,0 +1,38 @@ +////////////////////////////////////////////////////////////////////////////////////// +// This file is distributed under the University of Illinois/NCSA Open Source License. +// See LICENSE file in top directory for details. +// +// Copyright (c) 2025 QMCPACK developers. +// +// File developed by: Peter Doak, doakpw@ornl.gov, Oak Ridge National Laboratory +////////////////////////////////////////////////////////////////////////////////////// + +#ifndef QMCPLUSPLUS_VALIDWALKERLOGINPUT_H +#define QMCPLUSPLUS_VALIDWALKERLOGINPUT_H + +#include +#include +namespace qmcplusplus::testing +{ +class WalkerLogInputSections +{ +public: + enum class valid + { + DEFAULT = 0 + }; + + static std::string_view getXml(valid val) { return xml[static_cast(val)]; } + auto begin() { return xml.begin(); } + auto end() { return xml.end(); } + +private: + static constexpr std::array xml{ + R"XML( + +)XML"}; +}; + +} // namespace qmcplusplus::testing + +#endif diff --git a/src/QMCDrivers/tests/test_WalkerLogManager.cpp b/src/QMCDrivers/tests/test_WalkerLogManager.cpp new file mode 100644 index 0000000000..6c030919d7 --- /dev/null +++ b/src/QMCDrivers/tests/test_WalkerLogManager.cpp @@ -0,0 +1,65 @@ +////////////////////////////////////////////////////////////////////////////////////// +// This file is distributed under the University of Illinois/NCSA Open Source +// License. See LICENSE file in top directory for details. +// +// Copyright (c) 2025 QMCPACK developers. +// +// File developed by: Peter W. Doak, doakpw@ornl.gov, Oak Ridge National +// Laboratory +////////////////////////////////////////////////////////////////////////////////////// + +#include "OhmmsData/Libxml2Doc.h" +#include "QMCDrivers/WalkerLogManager.h" +#include "catch.hpp" + +#include "Message/Communicate.h" +#include "ValidWalkerLogInput.h" +#include "WalkerLogInput.h" + +namespace qmcplusplus +{ +class CollectorHolder +{ +public: + void setWalkerLogCollector(UPtr&& wlc) { walker_collector_ = std::move(wlc); } + void startBlock() { walker_collector_->startBlock(); } + +private: + UPtr walker_collector_; +}; + +struct LogAndStuff +{ + WalkerLogManager wlm; + CollectorHolder ch; +}; + +TEST_CASE("WalkerLogManager::move", "[drivers]") +{ + Communicate* comm = OHMMS::Controller; + + using WLInput = testing::WalkerLogInputSections; + Libxml2Document doc; + bool okay = doc.parseFromString(WLInput::getXml(WLInput::valid::DEFAULT)); + REQUIRE(okay); + xmlNodePtr node = doc.getRoot(); + WalkerLogInput walker_log_input{node}; + auto make_stuff = [comm](WalkerLogInput& walker_log_input) -> LogAndStuff { + WalkerLogManager wlm{walker_log_input, true, "root_name", comm}; + CollectorHolder ch; + ch.setWalkerLogCollector(wlm.makeCollector()); + return {std::move(wlm), std::move(ch)}; + }; + auto l_and_s = make_stuff(walker_log_input); + + // In the past this has resulted in a AddressSanitizer: + // stack-use-after-return + // due to access to a dead reference to + // a state object made back in moved from WalkerLogManager in + // the factory function. Seemed to work in release, caught by llvm + // asan. + l_and_s.ch.startBlock(); +} + + +} // namespace qmcplusplus