-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix formatting of types with custom addressof operator #2979
Conversation
There was a problem hiding this 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.
@@ -13,6 +13,7 @@ | |||
#include <cstring> // std::strlen | |||
#include <iterator> | |||
#include <limits> | |||
#include <memory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt/core.h
is designed to have minimal includes and <memory>
is not free (https://artificial-mind.net/projects/compile-health/). Can we avoid the include by using a cast (see e.g. https://en.cppreference.com/w/cpp/memory/addressof)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add a function to the detail
namespace. It won't be constexpr
though, but since it's not used for parsing I guess that won't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function that calls addressof
is constexpr
for C++14+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dammit, didn't notice that. I'm pretty sure that constexpr addressof
requires compiler support...
In Boost they detect the compiler and use a builtin function, would that be an option here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But std::addressof
is constexpr
only in C++17...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PeterFeicht did you find a way to format user types with custom operator&
? (I also have a wrapper type, running into the same scenario.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I completely forgot about that. We didn't yet have fmt v9 at that point so there was no format_as
. I'll do a PR when I have some time, then format_as
should work just fine for that purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Peter. Can you tag me in the PR if you remember?
Btw, when I look at the latest source it appear that this conditional change to be is_enum
or is_class
:
Line 1322 in 552c43a
FMT_ENABLE_IF(std::is_enum<U>::value || std::is_class<U>::value)> |
Compared to 9.10 that was is_enum
and is_integral
.
Line 1335 in a337011
FMT_ENABLE_IF(std::is_enum<U>::value&& std::is_integral<V>::value)> |
I updated to 10.0 yesterday, that includes that change. But that wasn't enough to allow a wrapper class with custom address-or. From this thread it appear that std::addressof
cannot be used due to compatibility?
(I'm curious to what the fix would be.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, didn't notice that. Now it should be enough to provide an overload of format_as
that maps to a wrapper type for formatting (which you would then provide a formatter
specialization for).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having another look at this. Does this mean there is no need for any changes to fmt, but that one can already implement formatting for wrapper types? I tried to add format_as
that return a string (for my string wrapping type) and then I tried to add a fmt::formatter
specialisation. But I got is not an entity that can be explicitly specialized
. (I'm not sure I fully understand how to correctly implement a specialisation.)
I have a type with a custom
operator&
that I want to format. That currently doesn't work, becausefmt
usesoperator&
to get avoid*
to a formatted object.This changes
fmt::detail::value
to usestd::addressof
instead, so types with an overloadedoperator&
will work.