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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__chrono/statically_widen.h:48
 
 #endif //if _LIBCPP_STD_VER > 17
 
----------------



================
Comment at: libcxx/include/__format/formatter.h:49-50
+
+#  endif //_LIBCPP_STD_VER > 20
 #endif //_LIBCPP_STD_VER > 17
 
----------------



================
Comment at: libcxx/include/__format/formatter_tuple.h:59
+    // TODO FMT this can be removed when P2733 is accepted.
+    __for_each_index_sequence(make_index_sequence<sizeof...(_Args)>(), [&]<size_t _Index> {
+      std::__set_debug_format(std::get<_Index>(__underlying_));
----------------
`std::`? Just for consistency.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:107
 /// explicitly.
 struct __fields {
   uint8_t __sign_ : 1 {false};
----------------
So then here we would also need to ABI-tag this. What we want is like `_LIBCPP_HIDE_FROM_ABI` but for types. I think it would do:

```
// like _LIBCPP_HIDE_FROM_ABI but without the __exclude_from_explicit_instantiation__
#define _LIBCPP_HIDE_TYPE_FROM_ABI _LIBCPP_HIDDEN __attribute__((__abi_tag__(_LIBCPP_TOSTRING(_LIBCPP_VERSIONED_IDENTIFIER))))
```

Can you add a TODO instead?


================
Comment at: libcxx/include/__utility/integer_sequence.h:141-144
+// Executes __func for every element in an index_sequence.
+inline constexpr auto __for_each_index_sequence = []<size_t... _Index>(index_sequence<_Index...>, auto __func) {
+  (__func.template operator()<_Index>(), ...);
+};
----------------
I would avoid using an inline variable here. They have the unfortunate side effect of creating a weak def for the variable itself, which is not really useful here. Instead, I would just use a normal function template. It'll also be more consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136775