Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable inheriting from formatter<std::string_view> #4055

Merged
merged 1 commit into from
Jul 14, 2024

Conversation

Arghnews
Copy link
Contributor

@Arghnews Arghnews commented Jul 7, 2024

Fixes #4036

This formatter specialization with base::format means a class implicitly convertible to std::string_view will now be converted by this format function before being passed to the fmt::string_view format function.
This wouldn't work previously as the compiler may only perform one implicit conversion, and we need 2 here (from our type, to std::string_view, to fmt::string_view).

@Arghnews Arghnews force-pushed the fmt_std_string_view_conversions branch from fd8e243 to a0d85e6 Compare July 7, 2024 16:38
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Comment on lines 3999 to 4008
// Fixes issue #4036
template <typename Char>
struct formatter<detail::std_string_view<Char>, Char>
: public formatter<basic_string_view<Char>, Char> {
auto format(const detail::std_string_view<Char>& value,
format_context& ctx) const -> decltype(ctx.out()) {
using base = formatter<basic_string_view<Char>, Char>;
return base::format(value, ctx);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like I forgot to actually comment what the requested change was =). Let's apply the change to FMT_FORMAT_AS rather than just the formatter specialization for string_view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, have updated review (haven't squashed yet so you can see diffs), with what I believe you're saying. Just check the types of the params and this is what you meant etc.

@Arghnews Arghnews force-pushed the fmt_std_string_view_conversions branch from a0d85e6 to 0b932de Compare July 13, 2024 04:22
template <typename Char> \
struct formatter<Type, Char> : formatter<Base, Char> { \
template <typename FormatContext> \
auto format(Type value, FormatContext&& ctx) const \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We either have to pass Type as a value like here, or turn this parameter into a forwarding reference too otherwise we get const errors. I opted for a value as the types for this are all small (<=16bytes)

Ditto on const issues with passing const format_context& or format_context& so have used a forwarding ref for that too (if there's a better way for this one let me know)

Copy link
Contributor

Choose a reason for hiding this comment

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

FormatContext should always be passed by a normal reference (&).

@Arghnews Arghnews requested a review from vitaut July 13, 2024 17:42
Allows (for example) types convertible to std::string_view to inherit
from the fmt::formatter<fmt::string_view> to work etc.
@Arghnews Arghnews force-pushed the fmt_std_string_view_conversions branch from 0980555 to 0c06809 Compare July 13, 2024 19:31
@vitaut vitaut merged commit 5ef93a9 into fmtlib:master Jul 14, 2024
43 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jul 14, 2024

Merged, thanks!

mtremer pushed a commit to ipfire/ipfire-2.x that referenced this pull request Aug 15, 2024
- Update from version 11.0.1 to 11.0.2
- Update of rootfile
- Changelog
    11.0.2
	- Fixed compatibility with non-POSIX systems
	  (fmtlib/fmt#4054,
	  fmtlib/fmt#4060).
	- Fixed performance regressions when using `std::back_insert_iterator` with
	  `fmt::format_to` (fmtlib/fmt#4070).
	- Fixed handling of `std::generator` and move-only iterators
	  (fmtlib/fmt#4053,
	  fmtlib/fmt#4057). Thanks @Arghnews.
	- Made `formatter<std::string_view>::parse` work with types convertible to
	  `std::string_view` (fmtlib/fmt#4036,
	  fmtlib/fmt#4055). Thanks @Arghnews.
	- Made `volatile void*` formattable
	  (fmtlib/fmt#4049,
	  fmtlib/fmt#4056). Thanks @Arghnews.
	- Made `Glib::ustring` not be confused with `std::string`
	  (fmtlib/fmt#4052).
	- Made `fmt::context` iterator compatible with STL algorithms that rely on
	  iterator category (fmtlib/fmt#4079).

Signed-off-by: Adolf Belka <[email protected]>
Signed-off-by: Michael Tremer <[email protected]>
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.

formatter simply inheriting from built-in formatter is not recognised
2 participants