[libcxx-commits] [PATCH] D129520: [libc++][ranges] implement `std::ranges::set_symmetric_difference`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_set_symmetric_difference.h:48
+      weakly_incrementable _OutIter,
+      class _Comp  = less,
+      class _Proj1 = identity,
----------------
Use `ranges::less` explicitly?


================
Comment at: libcxx/include/__algorithm/set_symmetric_difference.h:15
 #include <__algorithm/copy.h>
+#include <__algorithm/iterator_operations.h>
 #include <__config>
----------------
Nit: seems unused?


================
Comment at: libcxx/include/__algorithm/set_symmetric_difference.h:39
+template <class _Compare, class _InIter1, class _Sent1, class _InIter2, class _Sent2, class _OutIter>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 __set_symmetric_difference_result< _InIter1, _InIter2, _OutIter>
+__set_symmetric_difference(
----------------
Nit: this space looks weird -- is this done by `clang-format`?


================
Comment at: libcxx/include/__algorithm/set_symmetric_difference.h:76
+  typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
+  return std::__set_symmetric_difference<_Comp_ref>(__first1, __last1, __first2, __last2, __result, __comp).out;
 }
----------------
Move?


================
Comment at: libcxx/include/__algorithm/set_symmetric_difference.h:87
+  return std::set_symmetric_difference(
+      __first1,
+      __last1,
----------------
Move?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_symmetric_difference.pass.cpp:81
+
+static_assert(HasSetSymmetricDifferenceRange<UncheckedRange<int*>, UncheckedRange<int*>, int*>);
+
----------------
Optional: do you think a short alias like `template <class T> using R = UncheckedRange<T>` would make these checks a little more readable?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_symmetric_difference.pass.cpp:386
+
+    // member pointer Comparator iterator overload
+    {
----------------
Now that https://reviews.llvm.org/D129414 is merged, these tests (for member pointers) are no longer necessary.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_symmetric_difference.pass.cpp:444
+
+    // member pointer Projection iterator overload
+    {
----------------
Ditto -- these tests are no longer necessary.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_symmetric_difference.pass.cpp:479
+
+    // iterator overload
+    {
----------------
Optional: testing both overloads adds a lot of boilerplate. Consider:
a) make `numberOf*` members of a struct, move the definitions of `comp` and `proj1/proj2` out of the nested scopes, reset the struct (`numberOf = {}`) before each test case;
b) make the test a lambda taking a functor, invoke the lambda twice passing another lambda that invokes either overload as the functor.



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_symmetric_difference.pass.cpp:542
+  {
+    struct ConvertibleToBool {
+      bool b;
----------------
Ditto -- these tests are no longer necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129520