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

Jump to bottom

Range formatter #2983

Merged
merged 10 commits into from
Jul 29, 2022
Merged

Range formatter #2983

merged 10 commits into from
Jul 29, 2022

Conversation

brevzin
Copy link
Contributor

@brevzin brevzin commented Jul 12, 2022 •
edited
Loading

More complete implementation of P2286 and P2585 (renaming a couple things). For now this subsumes #2981.

This is still just the n specifier (no s, ?s, or m yet) but at least it's the full structure now.

@brevzin brevzin force-pushed the range-formatter branch 3 times, most recently from 1d10156 to 427496d Compare July 12, 2022 03:11
@brevzin
Copy link
Contributor Author

brevzin commented Jul 12, 2022

I'll be honest I don't understand the gcc 4.8 error:

/home/runner/work/fmt/fmt/include/fmt/ranges.h:324:43: error: conversion from ‘const char*’ to non-scalar type ‘fmt::v9::basic_string_view<char>’ requested

What's wrong with this constructor?

@mwinterb
Copy link
Contributor

Looks like it's just an "ancient GCC with new features" bug. If you explicitly provide a default constructor for formatter, it seems like it'll compile. https://gcc.godbolt.org/z/Wcosaqa6P. = default won't work. Also, if you move the initialization of separator_, etc to that constructor, it'll work, too.

@mwinterb
Copy link
Contributor

Also, to support the "esoteric" code unit types, I don't know if it'd be better to follow the time_point formatter model for specifying those three literals:

fmt/include/fmt/chrono.h

Lines 2020 to 2026 in 0db43cf

static constexpr const Char default_specs[] = {'%', 'F', ' ', '%', 'T'};
};
template <typename Char, typename Duration>
constexpr const Char
formatter<std::chrono::time_point<std::chrono::system_clock, Duration>,
Char>::default_specs[];

https://gcc.godbolt.org/z/q3KoTK9fK

@phprus
Copy link
Contributor

phprus commented Jul 12, 2022

default_specs[] without defined array size breaks Intel compiler (and other with EDG frontend).
See fix for this bug: #2982

@mwinterb
Copy link
Contributor

I was in a rush last night so I probably wasn't as clear as I should have been, but only formatter() {} would work.
However, you can also use brace initialization instead of assignment in the member initialization and that seems to be fine for 4.8: https://gcc.godbolt.org/z/E4boe4Gvr

vitaut
vitaut requested changes Jul 16, 2022
View reviewed changes
Copy link
Contributor

@vitaut vitaut left a 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. Please address build failures.

@brevzin
Copy link
Contributor Author

brevzin commented Jul 16, 2022

Thanks for the PR. Please address build failures.

Took me a while to figure this one out. Turns out this:

../include/fmt/core.h:2959:51: error: ‘constexpr decltype (ctx.begin()) fmt::v9::detail::parse_format_specs(ParseContext&) [with T = std::vector<int>; ParseContext = fmt::v9::detail::compile_parse_context<char, fmt::v9::detail::error_handler>; decltype (ctx.begin()) = const char*]’ called in a constant expression
 2959 |     return id >= 0 && id < num_args ? parse_funcs_[id](context_) : begin;
      |                                       ~~~~~~~~~~~~^

is a slightly insufficient error diagnostic for "oh, you added a default constructor that you forgot to mark constexpr".

@phprus
Copy link
Contributor

phprus commented Jul 16, 2022

@brevzin what do you think about comment #2983 (comment)?

The FMT_STATICALLY_WIDEN macro is limiting the xchar support, including the standard char16_t and char32_t types.

@vitaut
Copy link
Contributor

vitaut commented Jul 16, 2022 •
edited
Loading

I think conversion is a better approach than having redundant constants for exotic code unit types.

@brevzin
Copy link
Contributor Author

brevzin commented Jul 16, 2022

I think conversion is a better approach than having redundant constants for exotic code unit types.

Converting what, where?

@vitaut
Copy link
Contributor

vitaut commented Jul 16, 2022

I mean that instead of introducing a generic literal as suggested in #2983 (comment) we could have a normal string literal and convert it into wide and other strings when needed.

@phprus
Copy link
Contributor

phprus commented Jul 16, 2022

The implementation of the set_separator and set_brackets requires that string literals must immediately be of the correct character type.

phprus
phprus reviewed Jul 17, 2022
View reviewed changes
include/fmt/ostream.h Outdated Show resolved Hide resolved
vitaut
vitaut requested changes Jul 22, 2022
View reviewed changes
include/fmt/core.h Outdated Show resolved Hide resolved
include/fmt/ostream.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated
Comment on lines 438 to 440
basic_string_view<Char> separator_ = FMT_STATICALLY_WIDEN(Char, ", ");
basic_string_view<Char> opening_bracket_ = FMT_STATICALLY_WIDEN(Char, "[");
basic_string_view<Char> closing_bracket_ = FMT_STATICALLY_WIDEN(Char, "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do what was suggested in #2983 (comment) and remove FMT_STATICALLY_WIDEN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I think.

include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated
}

template <class U>
FMT_CONSTEXPR static void maybe_set_debug_format(U&, long) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

sneaky, we normally use ... for fallbacks like this.

phprus
phprus reviewed Jul 29, 2022
View reviewed changes
include/fmt/ranges.h Outdated Show resolved Hide resolved
vitaut
vitaut reviewed Jul 29, 2022
View reviewed changes
include/fmt/ranges.h Outdated Show resolved Hide resolved
@vitaut vitaut merged commit 0b2862a into fmtlib:master Jul 29, 2022
@vitaut
Copy link
Contributor

vitaut commented Jul 29, 2022

Thanks!

mtremer referenced this pull request in ipfire/ipfire-2.x Nov 28, 2022
@pmu-ipf
fmt: Update to version 9.1.0
90d43e5 
- Update from version 9.0.0 to 9.1.0
- Update of rootfile
- Changelog
    9.1.0 - 2022-08-27
	* ``fmt::formatted_size`` now works at compile time
		  `#3026 <https://github.com/fmtlib/fmt/pull/3026>`_
			  For example (`godbolt <https://godbolt.org/z/1MW5rMdf8>`__):
			   .. code:: c++
			     #include <fmt/compile.h>
			     int main() {
			       using namespace fmt::literals;
			       constexpr size_t n = fmt::formatted_size("{}"_cf, 42);
			       fmt::print("{}\n", n); // prints 2
			     }
	* Fixed handling of invalid UTF-8
		  `#3038 <https://github.com/fmtlib/fmt/pull/3038>`_,
		  `#3044 <https://github.com/fmtlib/fmt/pull/3044>`_,
		  `#3056 <https://github.com/fmtlib/fmt/pull/3056>`_
	* Improved Unicode support in ``ostream`` overloads of ``print``
		  `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_,
		  `#3001 <https://github.com/fmtlib/fmt/pull/3001>`_,
		  `#3025 <https://github.com/fmtlib/fmt/pull/3025>`_
	* Fixed handling of the sign specifier in localized formatting on systems with
	   32-bit ``wchar_t``
		  `#3041 <https://github.com/fmtlib/fmt/issues/3041>`_).
	* Added support for wide streams to ``fmt::streamed``
		  `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_
	* Added the ``n`` specifier that disables the output of delimiters when
	   formatting ranges
		  `#2981 <https://github.com/fmtlib/fmt/pull/2981>`_,
		  `#2983 <https://github.com/fmtlib/fmt/pull/2983>`_
			  For example (`godbolt <https://godbolt.org/z/roKqGdj8c>`__):
			   .. code:: c++
			     #include <fmt/ranges.h>
			     #include <vector>
			     int main() {
			       auto v = std::vector{1, 2, 3};
			       fmt::print("{:n}\n", v); // prints 1, 2, 3
			     }
	* Worked around problematic ``std::string_view`` constructors introduced in C++23
		  `#3030 <https://github.com/fmtlib/fmt/issues/3030>`_,
		  `#3050 <https://github.com/fmtlib/fmt/issues/3050>`_
	* Improve handling (exclusion) of recursive ranges
		  `#2968 <https://github.com/fmtlib/fmt/issues/2968>`_,
		  `#2974 <https://github.com/fmtlib/fmt/pull/2974>`_
	* Improved error reporting in format string compilation
		  `#3055 <https://github.com/fmtlib/fmt/issues/3055>`_
	* Improved the implementation of
		  `Dragonbox <https://github.com/jk-jeon/dragonbox>`_, the algorithm used for
		   the default floating-point formatting
		  `#2984 <https://github.com/fmtlib/fmt/pull/2984>`_
	* Fixed issues with floating-point formatting on exotic platforms.
	* Improved the implementation of chrono formatting
		  `#3010 <https://github.com/fmtlib/fmt/pull/3010>`_
	* Improved documentation
		  `#2966 <https://github.com/fmtlib/fmt/pull/2966>`_,
		  `#3009 <https://github.com/fmtlib/fmt/pull/3009>`_,
		  `#3020 <https://github.com/fmtlib/fmt/issues/3020>`_,
		  `#3037 <https://github.com/fmtlib/fmt/pull/3037>`_
	* Improved build configuration
		  `#2991 <https://github.com/fmtlib/fmt/pull/2991>`_,
		  `#2995 <https://github.com/fmtlib/fmt/pull/2995>`_,
		  `#3004 <https://github.com/fmtlib/fmt/issues/3004>`_,
		  `#3007 <https://github.com/fmtlib/fmt/pull/3007>`_,
		  `#3040 <https://github.com/fmtlib/fmt/pull/3040>`_
	* Fixed various warnings and compilation issues
		  `#2969 <https://github.com/fmtlib/fmt/issues/2969>`_,
		  `#2971 <https://github.com/fmtlib/fmt/pull/2971>`_,
		  `#2975 <https://github.com/fmtlib/fmt/issues/2975>`_,
		  `#2982 <https://github.com/fmtlib/fmt/pull/2982>`_,
		  `#2985 <https://github.com/fmtlib/fmt/pull/2985>`_,
		  `#2988 <https://github.com/fmtlib/fmt/issues/2988>`_,
		  `#3000 <https://github.com/fmtlib/fmt/issues/3000>`_,
		  `#3006 <https://github.com/fmtlib/fmt/issues/3006>`_,
		  `#3014 <https://github.com/fmtlib/fmt/issues/3014>`_,
		  `#3015 <https://github.com/fmtlib/fmt/issues/3015>`_,
		  `#3021 <https://github.com/fmtlib/fmt/pull/3021>`_,
		  `#3023 <https://github.com/fmtlib/fmt/issues/3023>`_,
		  `#3024 <https://github.com/fmtlib/fmt/pull/3024>`_,
		  `#3029 <https://github.com/fmtlib/fmt/pull/3029>`_,
		  `#3043 <https://github.com/fmtlib/fmt/pull/3043>`_,
		  `#3052 <https://github.com/fmtlib/fmt/issues/3052>`_,
		  `#3053 <https://github.com/fmtlib/fmt/pull/3053>`_,
		  `#3054 <https://github.com/fmtlib/fmt/pull/3054>`_

Signed-off-by: Adolf Belka <[email protected]>
Reviewed-by: Michael Tremer <[email protected]>
@roman-orekhov
Copy link

Is this intentionally undocumented in format syntax until other specifiers are implemented as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phprus phprus phprus left review comments

@vitaut vitaut vitaut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brevzin @mwinterb @phprus @vitaut @roman-orekhov