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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
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; });
----------------
ldionne wrote:
> Mordante wrote:
> > vitaut wrote:
> > > This is actually not tuple-like but pair-like so I suggest renaming to `__fmt_pair_like`.
> > Good point!
> Can you make sure to have a test that would fail if we were using the concept from P2165?
As discussed I added a TODO for that.


================
Comment at: libcxx/include/__format/range_default_formatter.h:54-55
+// instantiation. By using int as a type there's only one diagnostic.
+template <class _Rp>
+constexpr int format_kind = __instantiated_the_primary_template_of_format_kind<_Rp>{};
+
----------------
ldionne wrote:
> If that works, that would be a bit nicer and it would remove the need for the additional type + comment.
This works. Thanks for the suggestion!
(I changed the type back to `range_format` to avoid another error.)


================
Comment at: libcxx/include/__format/range_default_formatter.h:64
+  if constexpr (same_as<remove_cvref_t<ranges::range_reference_t<_Rp>>, _Rp>)
+    return range_format::disabled;
+  // 2.2 Otherwise, if the qualified-id R::key_type is valid and denotes a type:
----------------
ldionne wrote:
> Can you ensure that you have a test with a range where the reference type is the same as the range, without depending on filesystem? You should also keep your current test for `filesystem::path`.
I added a test to `libcxx/test/std/utilities/format/format.range/format.range.fmtkind/format_kind.compile.pass.cpp`.


================
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;
----------------
Mordante wrote:
> 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.
I discussed with @ldionne and he likes to do it in the same commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137271