Skip to content

Fmt wrappers - #3505

Open
tomc271 wants to merge 44 commits into
nextfrom
fmt-wrappers
Open

tomc271 wants to merge 44 commits into
nextfrom
fmt-wrappers

Conversation

@tomc271

@tomc271 tomc271 commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

Implement template specializations for the fmt library (or format_as() wrapper for enums).
This removes the need to call toString() on these types when passing them to BoutException.

so that fmt library will handle them automatically.
(by falling back to the default 'c' representation).
as fmt will handle it
(via the new `format_as` wrappers).
…fined (ODR)

Prevents fmt compilation issues.
so no need to call `toString()` explicitly.
(as it's needed for other functions, such as ::SQ).
… to bout_types.cxx

and add `toString` and `format_as()` to bout_types.hxx for the `const DirectionTypes&` type
to call its `toString()` method.
Not required as handled by fmt::formatter for SpecificInd.
Clang-Tidy: No header providing "UNUSED" is directly included
Not required as handled by fmt::formatter for SpecificInd.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread include/bout/output_bout_types.hxx
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread include/bout/output_bout_types.hxx

@ZedThree ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @tomc271, I didn't realise it was as simple as adding format_as, very nice!

As well as the comments below, a couple of other things:

  • could we flip the implementations, so that format_as has the actual implementation and toString just calls format_as?
  • then we can add [deprecated] to all toString definitions with a nice message about using fmt::format instead

Comment thread include/bout/bout_types.hxx Outdated
Comment on lines +98 to +99
std::string toString(const DirectionTypes& dir);
inline std::string format_as(const DirectionTypes& dir) { return toString(dir); }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It doesn't particularly matter, but could these be moved below the definition of DirectionTypes, and then we can drop the forward-decl on L31?

Comment thread include/bout/difops.hxx
Comment on lines +222 to +224
namespace bout {
inline std::string format_as(const ConductionMethod& method) { return toString(method); }
} // namespace bout

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we add this to the BOUT_ENUM_CLASS{,_NS} macros, then we don't need to add it manually

#include <type_traits>

template <IND_TYPE N>
struct fmt::formatter<SpecificInd<N>> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What was the issue that required this being moved?

The motivation for having it in this header, rather than region.hxx, was to make compilation cheaper -- this would only be included where needed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Just because having it in the same file prevented "unknown format specifier" errors at compile-time when the output_bout_types.hxx file isn't explicitly included
e.g.

include/bout/field.hxx: In instantiation of ‘void bout::checkFinite(const T&, const std::string&, const std::string&) [with T = Field2D; std::string = std::__cxx11::basic_string]’:
src/mesh/metric_tensor.cxx:33:20: required from here
include/bout/field.hxx:303:13: in ‘constexpr’ expansion of ‘fmt::v12::fstring<const std::__cxx11::basic_string<char, std::char_traits, std::allocator >&, SpecificInd<IND_TYPE::IND_2D>&>("{:s} is not finite at {:s}")’

Comment thread include/bout/region.hxx Outdated
struct fmt::formatter<SpecificInd<N>> {
char presentation = 'c';

// Parses format specifications: ['c' | 'i' | 's'] or empty {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does s do?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It was added to prevent this line failing:
throw BoutException("{:s} is not finite at {:s}", name, i);

The second :s here is a SpecificInd, so the parse() method of fmt::formatter<SpecificInd<N>> fails unless 's' is explicitly handled.

Perhaps the right solution is instead to change the calling code to:
throw BoutException("{:s} is not finite at {:c}", name, i);
or:
throw BoutException("{:s} is not finite at {:i}", name, i);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, that's because it has migrated from %s when we were using printf style formatting. Probably {:c} makes the most sense.

Comment thread include/bout/region.hxx Outdated
// Custom fmt formatter for SpecificInd
template <IND_TYPE N>
struct fmt::formatter<SpecificInd<N>> {
char presentation = 'c';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has lost the docstring about the meanings of c and i

which were accidentally deleted when moving.
…w its definition

so forward declaration is not needed.
@@ -135,30 +135,30 @@ struct DerivativeStore {
if (standard.count(key) != 0) {
throw BoutException("Trying to override standard derivative : "
"direction {:s}, stagger {:s}, key {:s}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"direction {:s}, stagger {:s}, key {:s}",
"direction {}, stagger {}, key {:s}",

I assume the s should always be removed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Enums using format_as support {:s} automatically, so it's only those types using a custom fmt::formatter<T> template specialisation (such as SpecificInd<N>) that may not handle :s
(because their custom parse() method defines the parsing of format presentation characters).

Even though not necessary, {} might still still be preferable though?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, nice to be consistent, then new developers looking at this are less likely to get tripped up

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/field/field.cxx
throw BoutException("{:s} is not allocated", name);
}

BOUT_FOR_SERIAL(i, f.getRegion(rgn)) {

Copy link
Copy Markdown
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 "BOUT_FOR_SERIAL" is directly included [misc-include-cleaner]

src/field/field.cxx:25:

- #include <bout/bout_types.hxx>
+ #include "bout/region.hxx"
+ #include <bout/bout_types.hxx>

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.

4 participants