[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!



================
Comment at: libcxx/include/__chrono/statically_widen.h:36
 #    define _LIBCPP_STATICALLY_WIDEN(_CharT, __str) ::std::__statically_widen<_CharT>(__str, L##__str)
-#  else // _LIBCPP_HAS_NO_WIDE_CHARACTERS
+#  else  // _LIBCPP_HAS_NO_WIDE_CHARACTERS
 
----------------
ldionne wrote:
> Stray whitespace?
No clang-format.


================
Comment at: libcxx/include/__format/formatter_tuple.h:118
+
+    [&]<size_t... __I>(index_sequence<__I...>) {
+      (
----------------
ldionne wrote:
> Here I would really suggest doing this to simplify things a bit:
> 
> ```
> auto __for_each = []<size_t ..._Index>(index_sequence<_Index...>, auto __func) {
>   (__func.operator()<_Index>(), ...);
> };
> 
> __for_each(make_index_sequence<sizeof...(_Args)>(), [&]<size_t _Index>() {
>   // current code
> });
> ```
> 
> This gets rid of one level of nesting. Also this `__for_each` function here should be extracted into a more general utility, since this has nothing specific to `format`.
> 
> Also it's unusual to use double-underscore with capital letter, so I'd stick to something more representative like `_Index`. And once you factor out `__for_each`, you don't need to number your indices anymore.
Looking at the code afterwards this is indeed an improvement, also the one above without the fold expression looks better.
Thanks for the suggestion.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:122
+
+  // TODO FMT We're now at 7 fields, should we increase the size for ABI reasons?
 };
----------------
ldionne wrote:
> > TODO FMT We're now at 7 fields, should we increase the size for ABI reasons?
> 
> I think instead we should find a way to make sure that this type doesn't need to be kept ABI-stable. I think we can probably achieve this by using an ABI tag on it, since this is never stored in a struct.
I've given it some more thought and I indeed think this is ABI save since it's only used for function arguments and these all use an ABI tag.


================
Comment at: libcxx/test/std/utilities/format/format.tuple/format.pass.cpp:7
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
----------------
ldionne wrote:
> Here and elsewhere, can you please add a synopsis of what you're testing?
Based on our discussion regarding D139561 I did a bit of renaming of these tests to make sure it's clear this tests the format function `std::format` and not the formatter's member function `format`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136775