-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Trying to improve errors in the unformattable case. #3478
Conversation
Having the type name displayed in diagnostic would be nice but unfortunately you can't use |
@brevzin, do you plan to update this PR? |
Yeah I will. Are you okay with a solution that just gives the better error in C++17, and maintains status quo before then? |
I am fine with that. |
Done. |
include/fmt/core.h
Outdated
#if defined(__cpp_if_constexpr) | ||
if constexpr (!formattable_char) { | ||
type_is_unformattable_for<T, typename Context::char_type> _; | ||
} | ||
#endif |
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.
I think the message below better explains the error, so let's remove this.
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.
You still get both errors - the static_assert
message is still printed. This way you also get which character type (and pointer type, for the next one) you were trying to print, which I think is still helpful info.
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.
Hmm, is this information actually useful? You cannot make the types in question formattable and I think having two errors may be confusing.
include/fmt/core.h
Outdated
#if defined(__cpp_if_constexpr) | ||
if constexpr (!formattable_pointer) { | ||
type_is_unformattable_for<T, typename Context::char_type> _; | ||
} | ||
#endif |
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.
Same here.
#if defined(__cpp_if_constexpr) | ||
if constexpr (std::is_default_constructible_v< | ||
formatter<mapped_type, char_type>>) { | ||
return formatter<mapped_type, char_type>().parse(ctx); | ||
} else { | ||
type_is_unformattable_for<T, char_type> _; | ||
return ctx.begin(); | ||
} | ||
#else |
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.
Why do we need to duplicate the logic here?
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.
So I could do something like this instead:
if constexpr (/* not default constructible */) {
type_is_unformattable_for<T, char_type> _;
}
return formatter<mapped_type, char_type>().parse(Ctx);
But doing it this way gives you both the good slightly better error message with type_is_unformattable
and also the "not default constructible" error message when you actually try to construct the thing. But all the latter one tells you is that... something failed, whereas the former tells you what failed. I think it's better to just provide the one?
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.
I am not sure why we care about checking default constructibility at all. It should be uncommon and not worth any extra diagnostics.
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.
I am not sure why we care about checking default constructibility at all. It should be uncommon and not worth any extra diagnostics.
Because that's what actually fails here. Today, if MyType
is not formattable and you try to format a MyType
, the error that you get is:
<source>:2549:10: error: use of deleted function 'fmt::v10::formatter<T, Char, Enable>::formatter() [with T = MyType; Char = char; Enable = void]'
2549 | return formatter<mapped_type, char_type>().parse(ctx);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
That's the only way of checking if something is formattable, as far as I'm aware. has_formatter
does the same thing.
Merged, thanks! |
This is my attempt to improve some some of the compile errors that we get when types aren't formattable at all.
Example 1:
Before - you know something is unformattable, but you don't know which one: is it the
X
, theY
, or theZ
?After: obviously the
Y
:Example 2, which is the more typical way people run into this:
Before (just copying the first error). Here, you can tell that
Y
isn't formattable, but the message is about something not being default constructible, which is a little cryptic?After: clearly
Y
is unformattable:Maybe it's possible to do better than this, I'm not sure, but this is the best I've been able to come up with so far at least.