[libcxx-commits] [PATCH] D132310: [libc++][regex] Uses operator<=> in sub_match.

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added a comment.

can you please add a new column to the status doc for the removal of the `operator!=` on `match_results`? So we don't forget about it...



================
Comment at: libcxx/docs/Status/SpaceshipProjects.csv:80
 | `[fs.dir.entry.obs] <https://wg21.link/fs.dir.entry.obs>`_,| `filesystem::directory_entry <https://reviews.llvm.org/D130860>`_,None,Adrian Vogelsgesang,|Complete|
-| `[re.submatch.op] <https://wg21.link/re.submatch.op>`_,| sub_match,None,Mark de Wever,|In Progress|
+| `[re.submatch.op] <https://wg21.link/re.submatch.op>`_,| sub_match,None,Mark de Wever,|Complete|
 | `[thread.thread.id] <https://wg21.link/thread.thread.id>`_,| `thread::id <https://reviews.llvm.org/D131362>`_,None,Adrian Vogelsgesang,|Complete|
----------------
please also add a link to this review


================
Comment at: libcxx/include/regex:368
+
+emplate <class BiIter, class ST, class SA>                                   // Removed in C++20
     bool
----------------



================
Comment at: libcxx/include/regex:5043
+template<class _BiIter>
+using __submatch_cat = compare_three_way_result_t<basic_string<typename iterator_traits<_BiIter>::value_type>>;
+template <class _BiIter>
----------------
avogelsgesang wrote:
> an empty line between the introduction of `__submatch_cat` and the `operator<=>` would make this a bit more readable
given the class is named `sub_match` instead of `submatch`, I would prefer the name `__sub_match_cat` here


================
Comment at: libcxx/include/regex:5043-5044
+template<class _BiIter>
+using __submatch_cat = compare_three_way_result_t<basic_string<typename iterator_traits<_BiIter>::value_type>>;
+template <class _BiIter>
+_LIBCPP_HIDE_FROM_ABI auto operator<=>(const sub_match<_BiIter>& __x, const sub_match<_BiIter>& __y) {
----------------
an empty line between the introduction of `__submatch_cat` and the `operator<=>` would make this a bit more readable


================
Comment at: libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp:224-225
+//     auto
+//     operator<=>(const sub_match<BiIter>& lhs,
+//                const basic_string<typename iterator_traits<BiIter>::value_type, ST, SA>& rhs);
+//
----------------
the parameters should align correctly below each other...


================
Comment at: libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp:229-230
+//     auto
+//     operator<=>(const sub_match<BiIter>& lhs,
+//                typename iterator_traits<BiIter>::value_type const* rhs);
+//
----------------



================
Comment at: libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp:247
 {
+#if TEST_STD_VER >17
+    AssertOrderReturn<std::strong_ordering, std::basic_string<CharT>>();
----------------
missing whitespace. Here and other places in this header


================
Comment at: libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp:248-251
+    AssertOrderReturn<std::strong_ordering, std::basic_string<CharT>>();
+#else
+    AssertCompareReturnBool<std::basic_string<CharT>>();
+#endif
----------------
shouldn't this rather check `sub_match`?
Move below the `typedef ... sub_match` and test for `sub_match`?


================
Comment at: libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp:262-267
     assert((sm1 == sm2) == (x == y));
     assert((sm1 != sm2) == (x != y));
     assert((sm1 < sm2) == (x < y));
     assert((sm1 > sm2) == (x > y));
     assert((sm1 <= sm2) == (x <= y));
     assert((sm1 >= sm2) == (x >= y));
----------------
replace by `testComparisons(sm1, sm2, /*isEqual*/ x == y, /*isLess*/ x <= y)`?

Same for the other remaining comparison tests


================
Comment at: libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp:339
 #endif
 
   return 0;
----------------
we should also add some test case for a character type which does not return a `strong_ordering`. Currently the `static_cast<__submatch_cat<_BiIter>>` is completely untested.

One way would be to use the same approach as in https://reviews.llvm.org/D132268 (search for "WeakComp"; "WeakComp" is introduced in https://reviews.llvm.org/D131395)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132310