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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 1, 2023
Merged

Trying to improve errors in the unformattable case. #3478

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b5bd632
Trying to improve errors in the unformattable case.
brevzin Jun 4, 2023
805e84f
Adding guards around if constexpr
brevzin Jun 19, 2023
9b2ae5e
Something something msvc.
brevzin Jun 19, 2023
422181f
Something something msvc.
brevzin Jun 19, 2023
1323cd4
Removing these two
brevzin Jun 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions include/fmt/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1539,23 +1539,43 @@ constexpr auto encode_types() -> unsigned long long {
(encode_types<Context, Args...>() << packed_arg_bits);
}

#if defined(__cpp_if_constexpr)
// This type is intentionally undefined, only used for errors
template <typename T, typename Char> struct type_is_unformattable_for;
#endif

template <bool PACKED, typename Context, typename T, FMT_ENABLE_IF(PACKED)>
FMT_CONSTEXPR FMT_INLINE auto make_arg(T& val) -> value<Context> {
using arg_type = remove_cvref_t<decltype(arg_mapper<Context>().map(val))>;

constexpr bool formattable_char =
!std::is_same<arg_type, unformattable_char>::value;
#if defined(__cpp_if_constexpr)
if constexpr (!formattable_char) {
type_is_unformattable_for<T, typename Context::char_type> _;
}
#endif
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

static_assert(formattable_char, "Mixing character types is disallowed.");

// Formatting of arbitrary pointers is disallowed. If you want to format a
// pointer cast it to `void*` or `const void*`. In particular, this forbids
// formatting of `[const] volatile char*` printed as bool by iostreams.
constexpr bool formattable_pointer =
!std::is_same<arg_type, unformattable_pointer>::value;
#if defined(__cpp_if_constexpr)
if constexpr (!formattable_pointer) {
type_is_unformattable_for<T, typename Context::char_type> _;
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

static_assert(formattable_pointer,
"Formatting of non-void pointers is disallowed.");

constexpr bool formattable = !std::is_same<arg_type, unformattable>::value;
#if defined(__cpp_if_constexpr)
if constexpr (!formattable) {
type_is_unformattable_for<T, typename Context::char_type> _;
}
#endif
static_assert(
formattable,
"Cannot format an argument. To make type T formattable provide a "
Expand Down Expand Up @@ -2520,7 +2540,17 @@ FMT_CONSTEXPR auto parse_format_specs(ParseContext& ctx)
mapped_type_constant<T, context>::value != type::custom_type,
decltype(arg_mapper<context>().map(std::declval<const T&>())),
typename strip_named_arg<T>::type>;
#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
Comment on lines +2533 to +2541
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

return formatter<mapped_type, char_type>().parse(ctx);
#endif
}

// Checks char specs and returns true iff the presentation type is char-like.
Expand Down