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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added a comment.

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.



================
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();
+}
----------------
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?


================
Comment at: libcxx/include/__format/formatter_tuple.h:63
+      } else
+        std::__throw_format_error("The tuple-format-spec type m requires two elements");
+    } else if (*__begin == _CharT('n')) {
----------------
nit: `tuple-format-spec` sounds too standardese and it's not super clear what elements the error refers to, I would suggest something along the lines of "The format specifier m requires a two-element tuple".


================
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)>());
----------------
This won't be called in case of an early return above.


================
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 '}'");
+
----------------
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.


================
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 {
----------------
Do you know why we do this conditional_t dance? For tuples with move-only elements?


================
Comment at: libcxx/include/__format/formatter_tuple.h:91
+  typename _FormatContext::iterator _LIBCPP_HIDE_FROM_ABI
+  __format(auto&& __tuple, _FormatContext& __ctx, __format_spec::__parsed_specifications<_CharT> __specs) const {
+    if (!__specs.__has_width())
----------------
This function looks unnecessary, I suggest merging into `format`.


================
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);
+
----------------
Maybe store the output iterator in a local variable and only call advance_to when you need to pass the context externally?


================
Comment at: libcxx/include/__format/formatter_tuple.h:135
+
+  __format_spec::__parser<_CharT> __parser_{.__alignment_ = __format_spec::__alignment::__left};
+
----------------
Why do you need to specify alignment here?


================
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, ", ");
----------------
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.


================
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?
----------------
Not related to this diff but `__type_` should probably be renamed to something more 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};
+
----------------
Maybe `__allow_colon_in_fill_` would be more descriptive?


================
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('}'))
----------------
Again, I would recommend avoiding standardese terms. Maybe something like "The fill format specifier contains an invalid 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}}
+
----------------
Out of curiosity, what does `@*:*` syntax mean here?


================
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);
+
----------------
Add a test case with a multibyte fill?


================
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);
+
----------------
Add a test case with invalid UTF-8?


================
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.
+
----------------
We should fix this.


================
Comment at: libcxx/test/std/utilities/format/format.tuple/pair.pass.cpp:34
+struct settings {
+  std::optional<std::basic_string_view<CharT>> separator{};
+  std::optional<std::pair<std::basic_string_view<CharT>, std::basic_string_view<CharT>>> brackets{};
----------------
nit: {} is redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136775