Skip to content

[libc++][format] Propagate m when formatting range elements#94562

Open
frederick-vs-ja wants to merge 1 commit intollvm:mainfrom
frederick-vs-ja:format-m
Open

[libc++][format] Propagate m when formatting range elements#94562
frederick-vs-ja wants to merge 1 commit intollvm:mainfrom
frederick-vs-ja:format-m

Conversation

@frederick-vs-ja
Copy link
Contributor

As per [tab:formatter.range.type], the effects of the m option need to be propagated to the formatter of range elements.

Fixes #60995. Fixes #90196.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner June 6, 2024 03:20
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2024

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

As per [tab:formatter.range.type], the effects of the m option need to be propagated to the formatter of range elements.

Fixes #60995. Fixes #90196.


Full diff: https://github.com/llvm/llvm-project/pull/94562.diff

2 Files Affected:

  • (modified) libcxx/include/__format/range_formatter.h (+2)
  • (modified) libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.tests.h (+3-3)
diff --git a/libcxx/include/__format/range_formatter.h b/libcxx/include/__format/range_formatter.h
index 6915630743493..ee94acc71776a 100644
--- a/libcxx/include/__format/range_formatter.h
+++ b/libcxx/include/__format/range_formatter.h
@@ -214,6 +214,8 @@ struct _LIBCPP_TEMPLATE_VIS range_formatter {
     switch (*__begin) {
     case _CharT('m'):
       if constexpr (__fmt_pair_like<_Tp>) {
+        __underlying_.set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ": "));
+        __underlying_.set_brackets({}, {});
         set_brackets(_LIBCPP_STATICALLY_WIDEN(_CharT, "{"), _LIBCPP_STATICALLY_WIDEN(_CharT, "}"));
         set_separator(_LIBCPP_STATICALLY_WIDEN(_CharT, ", "));
         ++__begin;
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 78b067e8cffa9..3d4a5db09b70f 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
@@ -993,10 +993,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<CharT> fmt : fmt_invalid_types<CharT>("s"))
@@ -1053,7 +1053,7 @@ void test_pair_tuple(TestFunction check, ExceptionTest check_exception, auto&& i
   check(SV("1: 'a', 42: '*'"), SV("{:n:m}"), input);
   check(SV("1, 'a', 42, '*'"), SV("{:n:n}"), input);
   check(SV("{1: 'a', 42: '*'}"), SV("{:m:m}"), input);
-  check(SV("{1, 'a', 42, '*'}"), SV("{:m:n}"), input);
+  check(SV("{1: 'a', 42: '*'}"), SV("{:m:n}"), input);
 }
 
 template <class CharT, class TestFunction, class ExceptionTest>

As per [tab:formatter.range.type], the effects of the `m` option need
to be propagated to the formatter of range elements.
@mordante
Copy link
Member

mordante commented Jul 5, 2024

Thanks for the patch. I haven't had time to investigate the example std::format("{:m:n}", std::vector{std::pair{0, "hello"}}); should do, which effectively sets both the option 'n' and 'm'. I hope to have time to look at it soon.

@ldionne
Copy link
Member

ldionne commented Jul 16, 2024

Tagging for LLVM 19 since I think this is a nice bug to fix.

@ldionne ldionne added this to the LLVM 19.X Release milestone Jul 16, 2024
@ldionne
Copy link
Member

ldionne commented Jul 23, 2024

Removing from LLVM 19 since this needs discussion on the reflector according to @mordante.

@ldionne ldionne removed this from the LLVM 19.X Release milestone Jul 23, 2024
@ldionne
Copy link
Member

ldionne commented Sep 10, 2024

@mordante Has there been any discussion on the reflector about this?

@frederick-vs-ja

This comment was marked as outdated.

@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Jun 3, 2025

Ping @mordante (again)

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

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[libc++][format] Formatting range with m range-type is incorrect <format>: The definition of __fmt_pair_like is wrong

4 participants