From af3cbd1e6108aea30fdee462ea64c0521ca0ad12 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Thu, 4 Dec 2025 16:28:01 +0000 Subject: [PATCH 1/3] Forward arguments to `BoutException` and `Output::write` This enables passing views like `fmt::join` which otherwise fail with: ``` error: static assertion failed: passing views as lvalues is disallowed ``` --- include/bout/boutexception.hxx | 15 ++++++---- include/bout/output.hxx | 43 ++++++++++++++------------- tests/unit/sys/test_boutexception.cxx | 12 +++++++- tests/unit/sys/test_output.cxx | 11 +++++++ 4 files changed, 54 insertions(+), 27 deletions(-) diff --git a/include/bout/boutexception.hxx b/include/bout/boutexception.hxx index 238c88654c..9c47eed86c 100644 --- a/include/bout/boutexception.hxx +++ b/include/bout/boutexception.hxx @@ -19,8 +19,9 @@ public: BoutException(std::string msg); template - BoutException(const S& format, const Args&... args) - : BoutException(fmt::format(format, args...)) {} + BoutException(S&& format, Args&&... args) + : BoutException(fmt::format(std::forward(format), + std::forward(args)...)) {} ~BoutException() override; @@ -46,16 +47,18 @@ class BoutRhsFail : public BoutException { public: BoutRhsFail(std::string message) : BoutException(std::move(message)) {} template - BoutRhsFail(const S& format, const Args&... args) - : BoutRhsFail(fmt::format(format, args...)) {} + BoutRhsFail(S&& format, Args&&... args) + : BoutRhsFail(fmt::format(std::forward(format), + std::forward(args)...)) {} }; class BoutIterationFail : public BoutException { public: BoutIterationFail(std::string message) : BoutException(std::move(message)) {} template - BoutIterationFail(const S& format, const Args&... args) - : BoutIterationFail(fmt::format(format, args...)) {} + BoutIterationFail(S&& format, Args&&... args) + : BoutIterationFail(fmt::format(std::forward(format), + std::forward(args)...)) {} }; #endif diff --git a/include/bout/output.hxx b/include/bout/output.hxx index 2862899067..124f170aea 100644 --- a/include/bout/output.hxx +++ b/include/bout/output.hxx @@ -42,6 +42,8 @@ class Output; #include "fmt/core.h" +#include + using std::endl; /// Class for text output to stdout and/or log file @@ -76,7 +78,8 @@ public: } template - Output(const S& format, const Args&... args) : Output(fmt::format(format, args...)) {} + Output(const S& format, Args&&... args) + : Output(fmt::format(format, std::forward(args)...)) {} ~Output() override { close(); } @@ -87,8 +90,8 @@ public: int open(const std::string& filename); template - int open(const S& format, const Args&... args) { - return open(fmt::format(format, args...)); + int open(const S& format, Args&&... args) { + return open(fmt::format(format, std::forward(args)...)); } /// Close the log file @@ -98,15 +101,15 @@ public: virtual void write(const std::string& message); template - void write(const S& format, const Args&... args) { - write(fmt::format(format, args...)); + void write(const S& format, Args&&... args) { + write(fmt::format(format, std::forward(args)...)); } /// Same as write, but only to screen virtual void print(const std::string& message); template - void print(const S& format, const Args&... args) { - print(fmt::format(format, args...)); + void print(const S& format, Args&&... args) { + print(fmt::format(format, std::forward(args)...)); } /// Add an output stream. All output will be sent to all streams @@ -136,14 +139,14 @@ private: class DummyOutput : public Output { public: template - void write([[maybe_unused]] const S& format, [[maybe_unused]] const Args&... args){}; + void write([[maybe_unused]] const S& format, [[maybe_unused]] Args&&... args) {}; template - void print([[maybe_unused]] const S& format, [[maybe_unused]] const Args&... args){}; - void write([[maybe_unused]] const std::string& message) override{}; - void print([[maybe_unused]] const std::string& message) override{}; - void enable() override{}; - void disable() override{}; - void enable([[maybe_unused]] bool enable){}; + void print([[maybe_unused]] const S& format, [[maybe_unused]] Args&&... args) {}; + void write([[maybe_unused]] const std::string& message) override {}; + void print([[maybe_unused]] const std::string& message) override {}; + void enable() override {}; + void disable() override {}; + void enable([[maybe_unused]] bool enable) {}; bool isEnabled() override { return false; } }; @@ -156,13 +159,13 @@ class ConditionalOutput : public Output { public: /// @param[in] base The Output object which will be written to if enabled /// @param[in] enabled Should this be enabled by default? - ConditionalOutput(Output* base, bool enabled = true) : base(base), enabled(enabled){}; + ConditionalOutput(Output* base, bool enabled = true) : base(base), enabled(enabled) {}; /// Constuctor taking ConditionalOutput. This allows several layers of conditions /// /// @param[in] base A ConditionalOutput which will be written to if enabled /// - ConditionalOutput(ConditionalOutput* base) : base(base), enabled(base->enabled){}; + ConditionalOutput(ConditionalOutput* base) : base(base), enabled(base->enabled) {}; /// If enabled, writes a string using fmt formatting /// by calling base->write @@ -170,10 +173,10 @@ public: void write(const std::string& message) override; template - void write(const S& format, const Args&... args) { + void write(const S& format, Args&&... args) { if (enabled) { ASSERT1(base != nullptr); - base->write(fmt::format(format, args...)); + base->write(fmt::format(format, std::forward(args)...)); } } @@ -182,10 +185,10 @@ public: void print(const std::string& message) override; template - void print(const S& format, const Args&... args) { + void print(const S& format, Args&&... args) { if (enabled) { ASSERT1(base != nullptr); - base->print(fmt::format(format, args...)); + base->print(fmt::format(format, std::forward(args)...)); } } diff --git a/tests/unit/sys/test_boutexception.cxx b/tests/unit/sys/test_boutexception.cxx index 39f8ee2145..322c7d6e42 100644 --- a/tests/unit/sys/test_boutexception.cxx +++ b/tests/unit/sys/test_boutexception.cxx @@ -4,8 +4,11 @@ #include "bout/boutexception.hxx" #include "gtest/gtest.h" -#include +#include + #include +#include +#include TEST(BoutExceptionTest, ThrowCorrect) { EXPECT_THROW(throw BoutException("test"), BoutException); @@ -45,6 +48,13 @@ TEST(BoutExceptionTest, GetBacktrace) { } } +TEST(BoutExceptionTest, FmtJoin) { + const std::vector things = {1, 2, 3, 4}; + constexpr std::string_view expected = "list: 1, 2, 3, 4"; + const BoutException exception{"list: {}", fmt::join(things, ", ")}; + EXPECT_EQ(exception.what(), expected); +} + TEST(BoutRhsFailTest, ThrowCorrect) { EXPECT_THROW(throw BoutRhsFail("RHS Fail test"), BoutRhsFail); } diff --git a/tests/unit/sys/test_output.cxx b/tests/unit/sys/test_output.cxx index 1338d9ce13..959823ede3 100644 --- a/tests/unit/sys/test_output.cxx +++ b/tests/unit/sys/test_output.cxx @@ -4,7 +4,10 @@ #include "bout/output_bout_types.hxx" #include "gtest/gtest.h" +#include + #include +#include // stdout redirection code from https://stackoverflow.com/a/4043813/2043465 class OutputTest : public ::testing::Test { @@ -385,3 +388,11 @@ TEST_F(OutputTest, FormatIndPerpi) { EXPECT_EQ(buffer.str(), "(15)"); } + +TEST_F(OutputTest, FmtJoin) { + const std::vector things = {1, 2, 3, 4}; + Output local_output; + local_output.write("list: {}", fmt::join(things, ", ")); + + EXPECT_EQ(buffer.str(), "list: 1, 2, 3, 4"); +} From e3f07907e06d555106fbe10d49fb9aa887ae2fc0 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Thu, 4 Dec 2025 18:08:45 +0000 Subject: [PATCH 2/3] Clang-tidy fixes --- include/bout/boutexception.hxx | 6 +++-- include/bout/output.hxx | 41 ++++++++++++++++++--------------- src/sys/boutexception.cxx | 42 +++++++++++++++++++--------------- src/sys/output.cxx | 14 ++++++++---- 4 files changed, 59 insertions(+), 44 deletions(-) diff --git a/include/bout/boutexception.hxx b/include/bout/boutexception.hxx index 9c47eed86c..ffd4a03a6c 100644 --- a/include/bout/boutexception.hxx +++ b/include/bout/boutexception.hxx @@ -16,6 +16,10 @@ void BoutParallelThrowRhsFail(int status, const char* message); class BoutException : public std::exception { public: + BoutException(const BoutException&) = default; + BoutException(BoutException&&) = delete; + BoutException& operator=(const BoutException&) = default; + BoutException& operator=(BoutException&&) = delete; BoutException(std::string msg); template @@ -39,8 +43,6 @@ private: int trace_size; char** messages; #endif - - void makeBacktrace(); }; class BoutRhsFail : public BoutException { diff --git a/include/bout/output.hxx b/include/bout/output.hxx index 124f170aea..205ca3a874 100644 --- a/include/bout/output.hxx +++ b/include/bout/output.hxx @@ -31,13 +31,11 @@ class Output; #include "bout/multiostream.hxx" #include -#include #include #include #include "bout/assert.hxx" -#include "bout/boutexception.hxx" -#include "bout/sys/gettext.hxx" // for gettext _() macro +#include "bout/sys/gettext.hxx" // IWYU pragma: keep for gettext _() macro #include "bout/unused.hxx" #include "fmt/core.h" @@ -63,14 +61,19 @@ using std::endl; class Output : private multioutbuf_init>, public std::basic_ostream> { - using _Tr = std::char_traits; - using multioutbuf_init = ::multioutbuf_init; + using Tr = std::char_traits; + using multioutbuf_init = ::multioutbuf_init; public: - Output() : multioutbuf_init(), std::basic_ostream(multioutbuf_init::buf()) { + Output() : multioutbuf_init(), std::basic_ostream(multioutbuf_init::buf()) { Output::enable(); } + Output(const Output&) = delete; + Output(Output&&) = delete; + Output& operator=(const Output&) = delete; + Output& operator=(Output&&) = delete; + /// Specify a log file to open Output(const std::string& filename) { Output::enable(); @@ -113,12 +116,10 @@ public: } /// Add an output stream. All output will be sent to all streams - void add(std::basic_ostream& str) { multioutbuf_init::buf()->add(str); } + void add(std::basic_ostream& str) { multioutbuf_init::buf()->add(str); } /// Remove an output stream - void remove(std::basic_ostream& str) { - multioutbuf_init::buf()->remove(str); - } + void remove(std::basic_ostream& str) { multioutbuf_init::buf()->remove(str); } static Output* getInstance(); ///< Return pointer to instance @@ -129,7 +130,7 @@ protected: private: std::ofstream file; ///< Log file stream - bool enabled; ///< Whether output to stdout is enabled + bool enabled{}; ///< Whether output to stdout is enabled }; /// Class which behaves like Output, but has no effect. @@ -219,8 +220,6 @@ private: /// The lower-level Output to send output to Output* base; -protected: - friend class WithQuietOutput; /// Does this instance output anything? bool enabled; }; @@ -279,18 +278,23 @@ ConditionalOutput& operator<<(ConditionalOutput& out, const T* t) { /// // output now enabled class WithQuietOutput { public: - explicit WithQuietOutput(ConditionalOutput& output_in) : output(output_in) { - state = output.enabled; - output.disable(); + WithQuietOutput(const WithQuietOutput&) = delete; + WithQuietOutput(WithQuietOutput&&) = delete; + WithQuietOutput& operator=(const WithQuietOutput&) = delete; + WithQuietOutput& operator=(WithQuietOutput&&) = delete; + explicit WithQuietOutput(ConditionalOutput& output_in) + : output(&output_in), state(output->isEnabled()) { + output->disable(); } - ~WithQuietOutput() { output.enable(state); } + ~WithQuietOutput() { output->enable(state); } private: - ConditionalOutput& output; + ConditionalOutput* output; bool state; }; +// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) /// To allow statements like "output.write(...)" or "output << ..." /// Output for debugging #ifdef BOUT_USE_OUTPUT_DEBUG @@ -306,5 +310,6 @@ extern ConditionalOutput output_verbose; ///< less interesting messages /// Generic output, given the same level as output_progress extern ConditionalOutput output; +// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) #endif // BOUT_OUTPUT_H diff --git a/src/sys/boutexception.cxx b/src/sys/boutexception.cxx index 825fb37e28..e81e395bf8 100644 --- a/src/sys/boutexception.cxx +++ b/src/sys/boutexception.cxx @@ -4,16 +4,19 @@ #include #include #include + #include #if BOUT_USE_BACKTRACE #include #include +#include #endif #include #include #include +#include #include @@ -22,18 +25,24 @@ const std::string header{"====== Exception thrown ======\n"}; } void BoutParallelThrowRhsFail(int status, const char* message) { - int allstatus; + int allstatus = 0; MPI_Allreduce(&status, &allstatus, 1, MPI_INT, MPI_LOR, BoutComm::get()); - if (allstatus) { + if (allstatus != 0) { throw BoutRhsFail(message); } } -BoutException::BoutException(std::string msg) : message(std::move(msg)) { - makeBacktrace(); +BoutException::BoutException(std::string msg) + : message(std::move(msg)) +#if BOUT_USE_BACKTRACE + , + trace_size(backtrace(trace.data(), TRACE_MAX)), + messages(backtrace_symbols(trace.data(), trace_size)) +#endif +{ if (std::getenv("BOUT_SHOW_BACKTRACE") != nullptr) { - message = getBacktrace() + "\n" + message; + message = fmt::format("{}\n{}", getBacktrace(), message); } } @@ -54,7 +63,8 @@ std::string BoutException::getBacktrace() const { backtrace_message = "====== Exception path ======\n"; // skip first stack frame (points here) for (int i = trace_size - 1; i > 1; --i) { - backtrace_message += fmt::format(FMT_STRING("[bt] #{:d} {:s}\n"), i - 1, messages[i]); + backtrace_message = fmt::format(FMT_STRING("{}[bt] #{:d} {:s}\n"), backtrace_message, + i - 1, messages[i]); // find first occurence of '(' or ' ' in message[i] and assume // everything before that is the file name. (Don't go beyond 0 though // (string terminator) @@ -65,11 +75,11 @@ std::string BoutException::getBacktrace() const { // If we are compiled as PIE, need to get base pointer of .so and substract Dl_info info; - void* ptr = trace[i]; - if (dladdr(trace[i], &info)) { + const void* ptr = trace[i]; + if (dladdr(ptr, &info) != 0) { // Additionally, check whether this is the default offset for an executable if (info.dli_fbase != reinterpret_cast(0x400000)) { - ptr = reinterpret_cast(reinterpret_cast(trace[i]) + ptr = reinterpret_cast(reinterpret_cast(ptr) - reinterpret_cast(info.dli_fbase)); } } @@ -82,14 +92,14 @@ std::string BoutException::getBacktrace() const { FILE* file = popen(syscom.c_str(), "r"); if (file != nullptr) { std::array out{}; - char* retstr = nullptr; + const char* retstr = nullptr; std::string buf; while ((retstr = fgets(out.data(), out.size() - 1, file)) != nullptr) { buf += retstr; } int const status = pclose(file); if (status == 0) { - backtrace_message += buf; + backtrace_message = fmt::format("{}{}", backtrace_message, buf); } } } @@ -97,12 +107,6 @@ std::string BoutException::getBacktrace() const { backtrace_message = "Stacktrace not enabled.\n"; #endif - return backtrace_message + msg_stack.getDump() + "\n" + header + message + "\n"; -} - -void BoutException::makeBacktrace() { -#if BOUT_USE_BACKTRACE - trace_size = backtrace(trace.data(), TRACE_MAX); - messages = backtrace_symbols(trace.data(), trace_size); -#endif + return fmt::format("{}{}\n{}{}\n", backtrace_message, msg_stack.getDump(), header, + message); } diff --git a/src/sys/output.cxx b/src/sys/output.cxx index c07cd53474..2c5e06bb7c 100644 --- a/src/sys/output.cxx +++ b/src/sys/output.cxx @@ -23,11 +23,13 @@ * **************************************************************************/ +#include #include -#include -#include -#include -#include + +#include + +#include +#include void Output::enable() { add(std::cout); @@ -68,7 +70,7 @@ void Output::close() { } void Output::write(const std::string& message) { - multioutbuf_init::buf()->sputn(message.c_str(), message.length()); + multioutbuf_init::buf()->sputn(message.c_str(), static_cast(message.length())); } void Output::print(const std::string& message) { @@ -98,6 +100,7 @@ void ConditionalOutput::print(const std::string& message) { } } +// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) #ifdef BOUT_USE_OUTPUT_DEBUG ConditionalOutput output_debug(Output::getInstance()); #else @@ -109,3 +112,4 @@ ConditionalOutput output_progress(Output::getInstance()); ConditionalOutput output_error(Output::getInstance()); ConditionalOutput output_verbose(Output::getInstance(), false); ConditionalOutput output(Output::getInstance()); +// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) From ab61b9e3c65f974ec5d0ac09ef7c6dd53f15b516 Mon Sep 17 00:00:00 2001 From: ZedThree <1486942+ZedThree@users.noreply.github.com> Date: Thu, 4 Dec 2025 18:12:08 +0000 Subject: [PATCH 3/3] Apply clang-format changes --- include/bout/output.hxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/bout/output.hxx b/include/bout/output.hxx index 205ca3a874..823e359753 100644 --- a/include/bout/output.hxx +++ b/include/bout/output.hxx @@ -160,13 +160,13 @@ class ConditionalOutput : public Output { public: /// @param[in] base The Output object which will be written to if enabled /// @param[in] enabled Should this be enabled by default? - ConditionalOutput(Output* base, bool enabled = true) : base(base), enabled(enabled) {}; + ConditionalOutput(Output* base, bool enabled = true) : base(base), enabled(enabled){}; /// Constuctor taking ConditionalOutput. This allows several layers of conditions /// /// @param[in] base A ConditionalOutput which will be written to if enabled /// - ConditionalOutput(ConditionalOutput* base) : base(base), enabled(base->enabled) {}; + ConditionalOutput(ConditionalOutput* base) : base(base), enabled(base->enabled){}; /// If enabled, writes a string using fmt formatting /// by calling base->write