Conversation
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.
|
Awesome, thanks @dschwoerer! Disappointing that we have to use |
| 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)), |
There was a problem hiding this comment.
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...)); |
There was a problem hiding this comment.
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...)); |
There was a problem hiding this comment.
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...)); |
There was a problem hiding this comment.
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)...)) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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]
| [&](Region<Ind2D> a, const std::string& b) { | |
| [&](const Region<Ind2D>& a, const std::string& b) { |
There was a problem hiding this comment.
Can we avoid this wrong comment? With C++20 it is moved.
There was a problem hiding this comment.
It might be that we need to compile with C++20 to avoid this
There was a problem hiding this comment.
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.
|
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.: |
| addRegion2D("RGN_BNDRY", std::accumulate(begin(boundary_names), end(boundary_names), | ||
| Region<Ind2D>{}, | ||
| [&](Region<Ind2D> a, const std::string& b) { | ||
| return a + getRegion2D(b); |
There was a problem hiding this comment.
I think we probably also need
| 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
No description provided.