[libcxx-commits] [PATCH] D136775: [libc++][format] Adds formatter for tuple and pair

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 11 inline comments as done.
Mordante added a comment.

Thanks for the review!

In D136775#3968626 <https://reviews.llvm.org/D136775#3968626>, @vitaut wrote:

> Overall looks good but we need to fix escaping in the standard and the implementation before this ships. Also a bunch of minor(ish) comments inline.

I too think we should fix escaping. I also think we should fix the nesting of formatters.
Normally we only implement papers after they have been voted in. That is a bit problematic since the next LLVM version will
branch a few weeks before the Issaquah meeting. I would like to see how the papers are received in LEWG, when positive I'll discuss with @ldionne which direction we want to take. (@ldionne I've seen the papers for both issues and I expect they will be in the next WG21 mailing.)



================
Comment at: libcxx/include/__format/formatter.h:45-46
+_LIBCPP_HIDE_FROM_ABI constexpr void __set_debug_format(_Tp& __formatter) {
+  if constexpr (requires { __formatter.set_debug_format(); })
+    __formatter.set_debug_format();
+}
----------------
vitaut wrote:
> Note that there is a paper in flight to change `set_debug_format` API. Do you see any problems with applying the change if the current diff lands?
That won't be an issue. I've implemented your paper P2733R0 locally, on top of a work-in-progress range-formatting branch.


================
Comment at: libcxx/include/__format/formatter_tuple.h:72-74
+    [&]<size_t... __I>(index_sequence<__I...>) {
+      (std::__set_debug_format(std::get<__I>(__underlying_)), ...);
+    }(std::make_index_sequence<sizeof...(_Args)>());
----------------
vitaut wrote:
> This won't be called in case of an early return above.
Nice catch. I didn't notice since that doesn't happen in practice since that only happens when the format string has no terminal '}'. So this version passes all tests.

I have moved it to be Standard conforming.


================
Comment at: libcxx/include/__format/formatter_tuple.h:77
+    if (__begin != __end && *__begin != _CharT('}'))
+      std::__throw_format_error("The format-spec should consume the input or end with a '}'");
+
----------------
vitaut wrote:
> Similarly, I suggest making this error message a bit more user-friendly e.g. "Unknown format specifier for tuple". Right now it is a bit too standardese (format-spec) and describing the implementation rather than the observable problem.
I use this message at other places too. Improving all error messages is still on my todo list.


================
Comment at: libcxx/include/__format/formatter_tuple.h:84
+  typename _FormatContext::iterator _LIBCPP_HIDE_FROM_ABI
+  format(conditional_t<(formattable<const _Args, _CharT> && ...), const _Tuple&, _Tuple&> __tuple,
+         _FormatContext& __ctx) const {
----------------
vitaut wrote:
> Do you know why we do this conditional_t dance? For tuples with move-only elements?
I haven't checked, why. But I too assume it's for move-only elements.


================
Comment at: libcxx/include/__format/formatter_tuple.h:119
+  _LIBCPP_HIDE_FROM_ABI typename _FormatContext::iterator __format_tuple(auto&& __tuple, _FormatContext& __ctx) const {
+    __ctx.advance_to(std::ranges::copy(__opening_bracket_, __ctx.out()).out);
+
----------------
vitaut wrote:
> Maybe store the output iterator in a local variable and only call advance_to when you need to pass the context externally?
While iterating over the underlying formatters we call `advance` every iteration. So I don't feel using a local variable helps a lot.


================
Comment at: libcxx/include/__format/formatter_tuple.h:135
+
+  __format_spec::__parser<_CharT> __parser_{.__alignment_ = __format_spec::__alignment::__left};
+
----------------
vitaut wrote:
> Why do you need to specify alignment here?
This is the default in the paper, therefore I set it explicitly.


================
Comment at: libcxx/include/__format/formatter_tuple.h:138
+private:
+  tuple<formatter<remove_cvref_t<_Args>, _CharT>...> __underlying_;
+  basic_string_view<_CharT> __separator_       = _LIBCPP_STATICALLY_WIDEN(_CharT, ", ");
----------------
vitaut wrote:
> I don't think you need this because the formatter is only used in the `format` function and can be constructed there. This will be more efficient and simpler.
I followed your suggestion, but unfortunately `__underlying_` is used in `parse`. It could be solved by calling `set_debug_format` in `format`. However that gives a change of behaviour; nested formatters will enable debug formatting. I've added a comment instead. This suggestion can be applied when implementing P2733R0.

(I prefer to keep the `underlying_` in the Standard since that makes it easier when we enable parsing for the underlying formatters.)


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:105
   uint8_t __locale_specific_form_ : 1 {false};
   uint8_t __type_ : 1 {false};
+  // Is the fill a range-fill?
----------------
vitaut wrote:
> Not related to this diff but `__type_` should probably be renamed to something more descriptive.
The names here match the names of the fields in the Standard. In that context I feel the name `__type_` is descriptive.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:111
+  // Note tuple-fill is the same as range-fill and has no separate type.
+  uint8_t __use_range_fill_ : 1 {false};
+
----------------
vitaut wrote:
> Maybe `__allow_colon_in_fill_` would be more descriptive?
I like that suggestion! I've updated the comments too.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:382
+        if (__use_range_fill && (*__begin == _CharT('{') || *__begin == _CharT('}') || *__begin == _CharT(':')))
+          std::__throw_format_error("The format-spec range-fill field contains an invalid character");
+        else if (*__begin == _CharT('{') || *__begin == _CharT('}'))
----------------
vitaut wrote:
> Again, I would recommend avoiding standardese terms. Maybe something like "The fill format specifier contains an invalid character"?
I will do that in a follow-up. Then I use the same message for both the "normal" and the "range" fill character.


================
Comment at: libcxx/test/std/utilities/format/format.tuple/format.verify.cpp:22
+  std::format("{::}", std::make_tuple(0)); // expected-error-re{{call to consteval function '{{.*}}' is not a constant expression}}
+  // expected-note@*:* {{non-constexpr function '__throw_format_error' cannot be used in a constant expression}}
+
----------------
vitaut wrote:
> Out of curiosity, what does `@*:*` syntax mean here?
This is a way to say "I don't care where the diagnostic is generated". This error checking is heavily used in Clang and LLVM where the message usually generated on the next line in the test. This error is generated in one of the format headers, using the exact location is a maintenance burden. Adding or removing a line it the header or refactoring headers will break this test. So in libc++ we use this style to reduce the maintenance in libc++. (We still run into test failures when the compiler changes its diagnostic, but that doesn't happen often.)


================
Comment at: libcxx/test/std/utilities/format/format.tuple/format_tests.h:58
+  check(SV("__(42, 99)___"), SV("{:_^13}"), input);
+  check(SV("#####(42, 99)"), SV("{:#>13}"), input);
+
----------------
vitaut wrote:
> Add a test case with a multibyte fill?
libc++ doesn't support multibyte fill for `char`.

I haven't implemented, not yet voted in paper, `P2572 std::format() fill character allowances` yet.


================
Comment at: libcxx/test/std/utilities/format/format.tuple/format_tests.h:157
+  std::get<0>(input) = CharT('\0');
+  check(SV(R"(('\u{0}', ""))"), SV("{}"), input);
+
----------------
vitaut wrote:
> Add a test case with invalid UTF-8?
I left thous out on purpose. Invalid Unicode sequences depend: on the type char/wchar_t, for wchar_t its size, and whether users have selected a library without Unicode support. These are tested in other places. Here I just test the basics of the escaping.


================
Comment at: libcxx/test/std/utilities/format/format.tuple/format_tests.h:302-303
+  //   std-format-spec parsed by the last call to parse were ?.
+  // pair and tuple are not debug-enabled specializations to the
+  // set_debug_format is not propagated.
+
----------------
vitaut wrote:
> We should fix this.
Agreed, I've added a comment regarding P2733.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136775/new/

https://reviews.llvm.org/D136775