[libcxx-commits] [PATCH] D137271: [libc++][format] Adds range-default-formatter.

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
Comment at: libcxx/include/__format/concepts.h:63-66
+// The paper P2165 "Compatibility between tuple, pair and tuple-like objects"
+// has a "tuple-like" concept, but that is not the same as this concept. That
+// paper doesn't affect the format header. Therefore use a different name for
+// the concept.
----------------
vitaut wrote:
> We definitely don't want to use the concept from P2165 because it includes array (maybe clarify this in the comment). The naming they took is very unfortunate but at least it's exposition-only.
Correct, I added comment to avoid possible confusion between the two concepts.


================
Comment at: libcxx/include/__format/concepts.h:68-70
+concept __fmt_tuple_like = __is_specialization_v<_Tp, pair> ||
+                           // Use a requires since tuple_size_v may fail to instantiate,
+                           (__is_specialization_v<_Tp, tuple> && requires { tuple_size_v<_Tp> == 2; });
----------------
vitaut wrote:
> This is actually not tuple-like but pair-like so I suggest renaming to `__fmt_pair_like`.
Good point!


================
Comment at: libcxx/include/__format/range_default_formatter.h:39-40
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wshadow")
+_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wshadow")
+enum class range_format { disabled, map, set, sequence, string, debug_string };
----------------
vitaut wrote:
> What does this shadow?
`set`, `map` and `string`. Not all of them now, but they will once the other specializations are implemented. Since it raises questions I've added a comment.


================
Comment at: libcxx/include/__format/range_default_formatter.h:57
+template <ranges::input_range _Rp>
+consteval range_format __get_format_format() {
+  // [format.range.fmtkind]/2
----------------
vitaut wrote:
> nit: `__get_format_format` looks a bit repetitive, maybe rename to `__get_range_format` (since it only applies to ranges) or something else? 
Good catch, it should have been `__get_range_format` in the first place.


================
Comment at: libcxx/include/__type_traits/is_specialization.h:35-39
+template <class _Tp, template <class...> class _Template>
+inline constexpr bool __is_specialization_v = false; // true if and only if _Tp is a specialization of _Template
+
+template <template <class...> class _Template, class... _Args>
+inline constexpr bool __is_specialization_v<_Template<_Args...>, _Template> = true;
----------------
vitaut wrote:
> This is not related to formatting and should probably be split into a separate diff.
I will do that before committing. Adding the trait without a use-case also looks odd.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137271