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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Looks pretty good to me, just a few comments.



================
Comment at: libcxx/include/__format/concepts.h:63
+
+// 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
----------------
Thanks for adding this comment! I would suggest linking to the section of the standard that defines that concept instead of the paper.


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


================
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>{};
+
----------------
If that works, that would be a bit nicer and it would remove the need for the additional type + comment.


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


================
Comment at: libcxx/include/__format/range_default_formatter.h:83
+  requires same_as<_Rp, remove_cvref_t<_Rp>>
+inline constexpr range_format format_kind<_Rp> = __get_range_format<_Rp>();
+
----------------
FWIW I wouldn't mind if you used an immediately-invoked lambda expression here. Not a request, just a suggestion.


================
Comment at: libcxx/include/__format/range_default_formatter.h:85-92
+// This is a non-standard work-around to fix instantiation of
+//   formatter<const _CharT[N], _CharT>
+// const _CharT[N] satisfies the ranges::input_range concept.
+// remove_cvref_t<const _CharT[N]> is _CharT[N] so it does not satisfy the
+// requirement of the above specialization. Instead it will instantiate the
+// primary template, which is ill-formed.
+//
----------------
After consideration, I'm fine with this. I'd much rather remove the offending formatter but let's wait for the LWG issue resolution before we do that.


================
Comment at: libcxx/include/__format/range_default_formatter.h:98
+// has been adopted.
+// TODO FMT Implement LWG38833.
+template <class _CharT, size_t N>
----------------



================
Comment at: libcxx/test/libcxx/type_traits/is_specialization.compile.pass.cpp:45
+
+// cvref _Tp is no specialization.
+static_assert(!std::__is_specialization_v<const std::pair<int, int>, std::pair>);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137271