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

Fix libfmt errors from not finding enum formatter #2022

Closed
wants to merge 1 commit into from

Conversation

mhx
Copy link
Contributor

@mhx mhx commented Jun 20, 2023

Recent versions of libfmt have become more strict and require enum types to be formattable:

static assertion failed due to requirement 'formattable': Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt

This is a quick fix to simply use the underlying type.

@mhx
Copy link
Contributor Author

mhx commented Jun 20, 2023

cc @ot as this was introduced in a42f818

@ot
Copy link
Contributor

ot commented Jun 20, 2023

Thanks for the fix, I think that fmt::underlying would be a terser option, but it's not clear from the documentation whether it's available in fmt/core.h or we need fmt/format.h.

@vitaut Do you have recommendations?

@mhx
Copy link
Contributor Author

mhx commented Jun 20, 2023

Thanks for the fix, I think that fmt::underlying would be a terser option [...]

Agreed, I just wasn't sure if this is already in libfmt 8.x which folly currently lists in its build manifest. At least libfmt 10.x defines underlying in fmt/format.h.

@vitaut
Copy link
Contributor

vitaut commented Jun 21, 2023

As documented, fmt::underlying is defined in fmt/format.h. You could also use std::to_underlying instead.

@mhx
Copy link
Contributor Author

mhx commented Jun 21, 2023

As documented, fmt::underlying is defined in fmt/format.h. You could also use std::to_underlying instead.

std::to_underlying is C++23, though, so I don't think this would make it into folly yet.

@ot, if fmt/format.h is too heavy, would you prefer to stick to static_cast for now?

@Orvid
Copy link
Contributor

Orvid commented Jun 21, 2023

Folly has folly::to_underlying which does this without the need to worry about C++ language versions, or where fmt::underlying lives. I would recommend just using that.

Recent versions of libfmt have become more strict and require
`enum` types to be formattable:

  static assertion failed due to requirement 'formattable': Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt

This is a quick fix to simply use the underlying type.
@mhx mhx force-pushed the mhx/fix-enum-formatting branch from d47eb44 to d783a64 Compare June 23, 2023 14:54
@mhx
Copy link
Contributor Author

mhx commented Jun 23, 2023

Updated PR using folly::to_underlying.

@facebook-github-bot
Copy link
Contributor

@ot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Jun 26, 2023
Summary:
Recent versions of libfmt have become more strict and require `enum` types to be formattable:

    static assertion failed due to requirement 'formattable': Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt

This is a quick fix to simply use the underlying type.

X-link: facebook/folly#2022

Reviewed By: yfeldblum

Differential Revision:
D46973532
Ninja: Flaky land-time

Pulled By: ot

fbshipit-source-id: 49b64e3885703c2157e66979e0523b5245a9470a
@facebook-github-bot
Copy link
Contributor

@ot merged this pull request in e74fe5c.

carlocab added a commit to carlocab/mvfst that referenced this pull request Jul 18, 2023
This change is patterned after facebook/folly#2022.

An alternative fix would be to provide a formatter specialisation[^1] for
`quic::LocalErrorCode`, but that's a bit more involved.

[^1]: See https://fmt.dev/10.0.0/api.html#udt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants