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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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.: |
tests/unit/fake_mesh.hxx
Outdated
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
|
I have a fix to get the compile-time format checking back: template <class... Args>
// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward)
void read(Options* options, fmt::format_string<Args...> format, const Args&... args) {
return read(options, fmt::vformat(format, fmt::make_format_args(args...)));
}I'll push a fix |
|
Bah, this doesn't play nice with the gettext |
This is probably needed 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.' Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
| num_y_processors, num_local_y_points, num_y_guards)}; | ||
| fmt::format(fmt::runtime(_( | ||
| "\t -> ny/NYPE ({:d}/{:d} = {:d}) must be >= MYG ({:d})\n")), | ||
| ny, num_y_processors, num_local_y_points, num_y_guards)}; |
There was a problem hiding this comment.
warning: no header providing "_" is directly included [misc-include-cleaner]
src/mesh/impls/bout/boutmesh.cxx:28:
- #include <bout/boundary_region.hxx>
+ #include "bout/sys/gettext.hxx"
+ #include <bout/boundary_region.hxx>| num_y_processors, num_local_y_points, num_y_guards)}; | ||
| fmt::format(fmt::runtime(_( | ||
| "\t -> ny/NYPE ({:d}/{:d} = {:d}) must be >= MYG ({:d})\n")), | ||
| ny, num_y_processors, num_local_y_points, num_y_guards)}; |
There was a problem hiding this comment.
warning: no header providing "fmt::format" is directly included [misc-include-cleaner]
src/mesh/impls/bout/boutmesh.cxx:27:
+ #include "fmt/format.h"| num_y_processors, num_local_y_points, num_y_guards)}; | ||
| fmt::format(fmt::runtime(_( | ||
| "\t -> ny/NYPE ({:d}/{:d} = {:d}) must be >= MYG ({:d})\n")), | ||
| ny, num_y_processors, num_local_y_points, num_y_guards)}; |
There was a problem hiding this comment.
warning: no header providing "fmt::runtime" is directly included [misc-include-cleaner]
src/mesh/impls/bout/boutmesh.cxx:27:
+ #include "fmt/base.h"| return {false, | ||
| fmt::format(fmt::runtime(_("\t -> Leg region jyseps1_1+1 ({:d}) must be a " | ||
| "multiple of MYSUB ({:d})\n")), | ||
| jyseps1_1 + 1, num_local_y_points)}; |
There was a problem hiding this comment.
warning: no header providing "fmt::format" is directly included [misc-include-cleaner]
fmt::format(fmt::runtime(_("\t -> Leg region jyseps1_1+1 ({:d}) must be a "
^| "be a multiple of MYSUB ({:d})\n"), | ||
| fmt::format(fmt::runtime(_( | ||
| "\t -> Core region jyseps2_1-jyseps1_1 ({:d}-{:d} = {:d}) must " | ||
| "be a multiple of MYSUB ({:d})\n")), |
There was a problem hiding this comment.
warning: no header providing "fmt::format" is directly included [misc-include-cleaner]
fmt::format(fmt::runtime(_(
^| Region<Ind2D>{}, | ||
| [&](Region<Ind2D> a, const std::string& b) { | ||
| return std::move(a) + getRegion2D(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]
| }) | |
| [&](const Region<Ind2D>& a, const std::string& b) { |
| [&](Region<Ind2D> a, const std::string& b) { | ||
| return std::move(a) + getRegion2D(b); | ||
| }) | ||
| .unique()); |
There was a problem hiding this comment.
warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
| .unique()); | |
| return a + getRegion2D(b); |
2 participants
No description provided.