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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Mostly LGTM but I would like to see again for `for_each`.



================
Comment at: libcxx/docs/Status/FormatPaper.csv:28
 `[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::sys_info``",A ``<chrono>`` implementation,Mark de Wever,,
 `[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::local_info``",A ``<chrono>`` implementation,Mark de Wever,,
 `[time.syn] <https://wg21.link/time.syn>`_,"Formatter ``chrono::zoned_time<Duration, TimeZonePtr>``",A ``<chrono>`` implementation,Mark de Wever,,
----------------
> 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.

If these issues were already in the pipeline and we knew pretty much which way things were going to go, I would say "sure, let's implement the expected issue resolution right away". However, it seems that we're a bit earlier than this here. I would recommend implementing whatever the standard says right now and have another patch ready to apply the expected issue resolution once it is discussed and we know which way we'll go.

We can also raise the unfortunate timing of the committee meeting and the LLVM release with the release manager -- perhaps it would be better for everyone to push back the LLVM branch point by 2 weeks or so.


================
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
 
----------------
Stray whitespace?


================
Comment at: libcxx/include/__format/formatter_tuple.h:57-58
+    //   is a valid expression, calls e.set_debug_format().
+    [&]<size_t... __I>(index_sequence<__I...>) {
+      (std::__set_debug_format(std::get<__I>(__underlying_)), ...);
+    }(std::make_index_sequence<sizeof...(_Args)>());
----------------
Let's use `__for_each` here too.


================
Comment at: libcxx/include/__format/formatter_tuple.h:118
+
+    [&]<size_t... __I>(index_sequence<__I...>) {
+      (
----------------
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.


================
Comment at: libcxx/include/__format/formatter_tuple.h:124-132
+          // During review Victor suggested to make the exposition only
+          // __underlying_ member a local variable. Currently the Standard
+          // requires nested debug-enabled formatter specializations not to
+          // output escaped output. P2733 fixes that bug, once accepted the
+          // code below can be used.
+          // (Note when a paper allows parsing a tuple-underlying-spec the
+          // exposition only member needs to be a class member. Earlier
----------------
Can you add a `TODO(format)` here and also above where we currently set the debug format? IIUC the above one needs to be removed when we use this `#if 0` branch.


================
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?
 };
----------------
> 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.


================
Comment at: libcxx/test/std/utilities/format/format.tuple/format.pass.cpp:7
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
----------------
Here and elsewhere, can you please add a synopsis of what you're testing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136775