Skip to content

Get BOUT++ ready for C++20#3264

Open
dschwoerer wants to merge 4 commits intonextfrom
c++20-ready
Open

Get BOUT++ ready for C++20#3264
dschwoerer wants to merge 4 commits intonextfrom
c++20-ready

Conversation

@dschwoerer
Copy link
Contributor

No description provided.

Otherwise fmt tries to do compile-time checks, but fails as it cannot be
checked at compile time.
fmtlib/fmt#4179
With C++ std::accumulate uses std::move, which fails with the reference.
isatty is provided by both cpptrace and unistd.h, so we should only
include one.
@ZedThree
Copy link
Member

ZedThree commented Feb 3, 2026

Awesome, thanks @dschwoerer! Disappointing that we have to use fmt::runtime in many places. It might be the case that we can make at least some of them constexpr instead?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

template <class S, class... Args>
BoutException(S&& format, Args&&... args)
: BoutException(fmt::format(std::forward<S>(format),
: BoutException(fmt::format(fmt::runtime(std::forward<S>(format)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::runtime" is directly included [misc-include-cleaner]

include/bout/boutexception.hxx:7:

- #include "fmt/core.h"
+ #include "fmt/base.h"
+ #include "fmt/core.h"

template <class S, class... Args>
int push(const S& format, const Args&... args) {
return push(fmt::format(format, args...));
return push(fmt::format(fmt::runtime(format), args...));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

    return push(fmt::format(fmt::runtime(format), args...));
                                         ^

template <class S, class... Args>
int push(const S& format, const Args&... args) {
return push(fmt::format(format, args...));
return push(fmt::format(fmt::runtime(format), args...));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::runtime" is directly included [misc-include-cleaner]

include/bout/msg_stack.hxx:26:

- class MsgStack;
+ #include "fmt/base.h"
+ class MsgStack;

template <class S, class... Args>
void read(Options* options, const S& format, const Args&... args) {
return read(options, fmt::format(format, args...));
return read(options, fmt::format(fmt::runtime(format), args...));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::runtime" is directly included [misc-include-cleaner]

include/bout/optionsreader.hxx:31:

- class OptionsReader;
+ #include "fmt/base.h"
+ class OptionsReader;

template <class S, class... Args>
Output(const S& format, Args&&... args)
: Output(fmt::format(format, std::forward<decltype(args)>(args)...)) {}
: Output(fmt::format(fmt::runtime(format), std::forward<decltype(args)>(args)...)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::runtime" is directly included [misc-include-cleaner]

include/bout/output.hxx:25:

- class Output;
+ #include "fmt/base.h"
+ class Output;

for (const auto& child : children) {
if (child.second.isValue()) {
fmt::format_to(ctx.out(), format_string, child.second);
fmt::format_to(ctx.out(), fmt::runtime(format_string), child.second);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::format_to" is directly included [misc-include-cleaner]

      fmt::format_to(ctx.out(), fmt::runtime(format_string), child.second);
           ^


// Call recursive function to write to file
fout << fmt::format("{:uds}", *options);
fout << fmt::format(fmt::runtime("{:uds}"), *options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::format" is directly included [misc-include-cleaner]

src/sys/options/options_ini.cxx:52:

+ #include "fmt/format.h"


// Call recursive function to write to file
fout << fmt::format("{:uds}", *options);
fout << fmt::format(fmt::runtime("{:uds}"), *options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "fmt::runtime" is directly included [misc-include-cleaner]

src/sys/options/options_ini.cxx:52:

+ #include "fmt/base.h"

.unique());
addRegion2D("RGN_BNDRY", std::accumulate(begin(boundary_names), end(boundary_names),
Region<Ind2D>{},
[&](Region<Ind2D> a, const std::string& b) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: the parameter 'a' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change
[&](Region<Ind2D> a, const std::string& b) {
[&](const Region<Ind2D>& a, const std::string& b) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this wrong comment? With C++20 it is moved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be that we need to compile with C++20 to avoid this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this is merged, fedora-ci will switch to gcc16, which will compile as C++20, so will notice at least if we break it.

@dschwoerer dschwoerer mentioned this pull request Feb 3, 2026
@dschwoerer
Copy link
Contributor Author

I have not tested whether constexpr is possible. I just changed everything to fmt::runtime, where the compiler complained.

I need this to get BOUT++ building for fedora again, see e.g.:
https://github.com/dschwoerer/bout-container-base/actions/runs/21592270382/job/62328541345
https://copr.fedorainfracloud.org/coprs/davidsch/testing/build/10086815/

addRegion2D("RGN_BNDRY", std::accumulate(begin(boundary_names), end(boundary_names),
Region<Ind2D>{},
[&](Region<Ind2D> a, const std::string& b) {
return a + getRegion2D(b);
Copy link
Member

@ZedThree ZedThree Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably also need

Suggested change
return a + getRegion2D(b);
return std::move(a) + getRegion2D(b);

(and on L245 below) to take advantage of C++20's std::move in accumulate. Although it gets moved in, our use here is still an l-value.

std::move is only a cast, so this should always be correct and also keep clang-tidy happy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants