[libcxx-commits] [PATCH] D114912: [libc++] [P1614] Hidden-friend operator<=> for string_view.
Joe Loser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Dec 1 17:38:13 PST 2021
jloser added inline comments.
================
Comment at: libcxx/include/__compare/comp_cat.h:27
+
+template<class _Traits, class _Dflt, class = void>
+struct __comp_cat {
----------------
What's `_Dflt` short for? Ditto below in the other alias templates.
================
Comment at: libcxx/include/__compare/comp_cat.h:33
+template<class _Traits, class _Dflt>
+struct __comp_cat<_Traits, _Dflt, typename __void_t<typename _Traits::comparison_category>::type> {
+ using type = typename _Traits::comparison_category;
----------------
Do we need to use `void_t` idiom if this is C++20-or-later code (can we use `requires`?)?
================
Comment at: libcxx/include/string_view:29
// 7.9, basic_string_view non-member comparison functions
+ // We make these hidden friends, in lieu of "sufficient additional overloads"
template<class charT, class traits>
----------------
Should we mention the note here that this is non-conforming? :)
================
Comment at: libcxx/include/string_view:728
inline constexpr bool ranges::enable_borrowed_range<basic_string_view<_CharT, _Traits> > = true;
#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
----------------
We can probably drop the comment here given how it's just 6 lines up. Note in the next `#if` block below (`#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES))`, we omit the comment since it's short like this one.
================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:13
+
+// template<class charT, class traits>
+// constexpr auto operator<=>(basic_string_view<charT, traits> lhs, basic_string_view<charT, traits> rhs);
----------------
Nit: `s/charT/CharT` and `s/traits/Traits` to be consistent with `libcxx` code in `string_view`?
================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/spaceship.pass.cpp:36
+template<class SV, class OrderingT>
+TEST_CONSTEXPR_CXX14 bool test()
+{
----------------
Since this test is run in C++20-or-later only, let's `s/TEST_CONSTEXPR_CXX14/constexpr/g`, please.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114912/new/
https://reviews.llvm.org/D114912
More information about the libcxx-commits
mailing list