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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
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.
----------------
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.


================
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; });
----------------
This is actually not tuple-like but pair-like so I suggest renaming to `__fmt_pair_like`.


================
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 };
----------------
What does this shadow?


================
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
----------------
nit: `__get_format_format` looks a bit repetitive, maybe rename to `__get_range_format` (since it only applies to ranges) or something else? 


================
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;
----------------
This is not related to formatting and should probably be split into a separate diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137271