From d590a6533652e50669accb2260b70ab25788270b Mon Sep 17 00:00:00 2001 From: Yihe Li Date: Mon, 30 Oct 2023 12:19:51 +0800 Subject: [PATCH 1/3] Fix m specifier for range formatting: call set_bracket/separator on underlying too --- libcxx/include/__format/range_formatter.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libcxx/include/__format/range_formatter.h b/libcxx/include/__format/range_formatter.h index d13278009fcf8..25d0f5697c49c 100644 --- a/libcxx/include/__format/range_formatter.h +++ b/libcxx/include/__format/range_formatter.h @@ -216,6 +216,8 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter { if constexpr (__fmt_pair_like<_Tp>) { set_brackets(_LIBCPP_STATICALLY_WIDEN(_CharT, "{"), _LIBCPP_STATICALLY_WIDEN(_CharT, "}")); set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ", ")); + __underlying_.set_brackets({}, {}); + __underlying_.set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ": ")); ++__begin; } else std::__throw_format_error("Type m requires a pair or a tuple with two elements"); From 5bdf2c2c7961881ece04a1b943d3331c566f2ec4 Mon Sep 17 00:00:00 2001 From: Yihe Li Date: Mon, 30 Oct 2023 13:49:03 +0800 Subject: [PATCH 2/3] Additional bugfix: only add m specifier when there is no underlying spec & change test suite --- libcxx/include/__format/range_formatter.h | 16 ++++++++++++---- .../format.functions.tests.h | 4 ++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/libcxx/include/__format/range_formatter.h b/libcxx/include/__format/range_formatter.h index 25d0f5697c49c..d58742332d090 100644 --- a/libcxx/include/__format/range_formatter.h +++ b/libcxx/include/__format/range_formatter.h @@ -65,7 +65,7 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter { // The n field overrides a possible m type, therefore delay applying the // effect of n until the type has been procesed. - __parse_type(__begin, __end); + bool __has_m_specifier = __parse_type(__begin, __end); if (__parser_.__clear_brackets_) set_brackets({}, {}); if (__begin == __end) [[unlikely]] @@ -87,6 +87,14 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter { __ctx.advance_to(__begin); __begin = __underlying_.parse(__ctx); + // Add "m" specifier semantics if there are no underlying specifier + if (!__has_range_underlying_spec && __has_m_specifier) { + if constexpr (__fmt_pair_like<_Tp>) { // should always be true + __underlying_.set_brackets({}, {}); + __underlying_.set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ": ")); + } + } + // This test should not be required if __has_range_underlying_spec is false. // However this test makes sure the underlying formatter left the parser in // a valid state. (Note this is not a full protection against evil parsers. @@ -210,15 +218,14 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter { private: template - _LIBCPP_HIDE_FROM_ABI constexpr void __parse_type(_Iterator& __begin, _Iterator __end) { + _LIBCPP_HIDE_FROM_ABI constexpr bool __parse_type(_Iterator& __begin, _Iterator __end) { switch (*__begin) { case _CharT('m'): if constexpr (__fmt_pair_like<_Tp>) { set_brackets(_LIBCPP_STATICALLY_WIDEN(_CharT, "{"), _LIBCPP_STATICALLY_WIDEN(_CharT, "}")); set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ", ")); - __underlying_.set_brackets({}, {}); - __underlying_.set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ": ")); ++__begin; + return true; } else std::__throw_format_error("Type m requires a pair or a tuple with two elements"); break; @@ -241,6 +248,7 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter { } else std::__throw_format_error("Type ?s requires character type as formatting argument"); } + return false; } template diff --git a/libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.tests.h b/libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.tests.h index 1e6c75cc69fe4..2417efe4afe3d 100644 --- a/libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.tests.h +++ b/libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.tests.h @@ -983,10 +983,10 @@ void test_pair_tuple(TestFunction check, ExceptionTest check_exception, auto&& i // *** n check(SV("__(1, 'a'), (42, '*')___"), SV("{:_^24n}"), input); - check(SV("__(1, 'a'), (42, '*')___"), SV("{:_^24nm}"), input); // m should have no effect + check(SV("____1: 'a', 42: '*'_____"), SV("{:_^24nm}"), input); // *** type *** - check(SV("__{(1, 'a'), (42, '*')}___"), SV("{:_^26m}"), input); + check(SV("____{1: 'a', 42: '*'}_____"), SV("{:_^26m}"), input); check_exception("Type s requires character type as formatting argument", SV("{:s}"), input); check_exception("Type ?s requires character type as formatting argument", SV("{:?s}"), input); for (std::basic_string_view fmt : fmt_invalid_types("s")) From 00f09851888ab2831332ae3d43b4393793eb1276 Mon Sep 17 00:00:00 2001 From: Yihe Li Date: Mon, 30 Oct 2023 13:52:33 +0800 Subject: [PATCH 3/3] Change tab width to 2 --- libcxx/include/__format/range_formatter.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libcxx/include/__format/range_formatter.h b/libcxx/include/__format/range_formatter.h index d58742332d090..76bd9ca5455d6 100644 --- a/libcxx/include/__format/range_formatter.h +++ b/libcxx/include/__format/range_formatter.h @@ -89,10 +89,10 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter { // Add "m" specifier semantics if there are no underlying specifier if (!__has_range_underlying_spec && __has_m_specifier) { - if constexpr (__fmt_pair_like<_Tp>) { // should always be true - __underlying_.set_brackets({}, {}); - __underlying_.set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ": ")); - } + if constexpr (__fmt_pair_like<_Tp>) { // should always be true + __underlying_.set_brackets({}, {}); + __underlying_.set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ": ")); + } } // This test should not be required if __has_range_underlying_spec is false.