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

Add support for 'std::variant' in C++17 #2941

Merged
merged 6 commits into from
Jun 26, 2022

Conversation

jehelset
Copy link
Contributor

@jehelset jehelset commented Jun 18, 2022

Quick variant implementation based on same machinery as tuple-formatting in #2940.

I just wanted to submit it to see if it was acceptable.

@jehelset jehelset changed the title Add support for Add support for 'std::variant' in C++17 Jun 18, 2022
@phprus
Copy link
Contributor

phprus commented Jun 19, 2022

My suggestions:

Move variant.h content into std.h.
Use #ifdef __cpp_lib_variant instad of #if FMT_HAS_VARIANT.

For C++17, if all the alternatives of a variant are formattable
the variant is now also formattable. In addition 'std::monostate'
is now formattable.

The value of a variant is enclosed in '<' and '>', and the monostate
is formatted as ' '.
@vitaut
Copy link
Contributor

vitaut commented Jun 20, 2022

Thanks for the PR. This should definitely go to fmt/std.h and please fix build failures.

jehelset added 2 commits June 22, 2022 06:06
Moves implementation into 'std.h', and tests into 'std-test.cc'.

Avoid fold-expression since MSVC was crashing. Hopefully works
with this.

Add secion for 'fmt/std.h' in API-docs.
@jehelset
Copy link
Contributor Author

i rewrote write_range_entry from fmt/range.h as write_variant_alternative. since variant is C++17, i hoped i can could get away with if-constexpr instead, and compilers didn't complain. i just used is_string from fmt/core.h instead of this is_std_string_like in fmt/ranges.h, to not have to include or move code around. i also took the liberty of using std::integral_sequence, std::void_t and std::conjuction, and instead of checking for those library features i used the __cpp_lib_variant as a proxy. that's not quite correct, but i'm not sure what is the best approach.

doc/api.rst Outdated Show resolved Hide resolved
include/fmt/std.h Outdated Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
include/fmt/std.h Outdated Show resolved Hide resolved
include/fmt/std.h Outdated Show resolved Hide resolved
@vitaut
Copy link
Contributor

vitaut commented Jun 23, 2022

Overall looks good but I'd suggest changing the default representation to variant(...). Also a few comments inline.

@jehelset
Copy link
Contributor Author

Overall looks good but I'd suggest changing the default representation to variant(...). Also a few comments inline.

If we can be verbose I would prefer to print std::monostate as monostate, and a variant holding a monostate as just variant(monostate). Probably could do with some format specifiers, but I will leave that for someone more proficient.

@vitaut vitaut merged commit 6a775e9 into fmtlib:master Jun 26, 2022
@vitaut
Copy link
Contributor

vitaut commented Jun 26, 2022

Thank you!

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.

3 participants