[libcxx-commits] [PATCH] D129233: [libc++][ranges] implement `std::ranges::set_intersection`

[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/docs/Status/RangesAlgorithms.csv:57
 Write,swap_ranges,Nikolas Klauser,`D116303 <https://llvm.org/D116303>`_,✅
 Write,reverse_copy,Nikolas Klauser,`D127211 <https://llvm.org/D127211>`_,Under review
 Write,rotate_copy,Nikolas Klauser,`D127211 <https://llvm.org/D127211>`_,Under review
----------------
Nit: `s/set_intersenction/set_intersection/` in the patch description.


================
Comment at: libcxx/include/__algorithm/set_intersection.h:32
+#else
+    tuple< __uncvref_t<_InIter1>,
+           __uncvref_t<_Sent1>,
----------------
I'd suggest defining a simple struct to hold the return values instead. It would be more readable than a tuple (the meaning of e.g. `ret.first1` is a lot more obvious than `std::get<0>(ret)`) and would avoid the need for the conditional macros.


================
Comment at: libcxx/include/__algorithm/set_intersection.h:67
+      __uncvref_t<_OutIter>>{
+      std::move(__first1), std::move(__last1), std::move(__first2), std::move(__last2), std::move(__result)};
+#endif
----------------
Instead of returning `__first1` and `__first2` and putting the responsibility to convert sentinels to iterators on the caller, I'd suggest calling `next` here and returning only `__first1` (advanced to the last position), `__first2` (same) and `__result`. I think `iterator_operations.h` would be a good place to hide the difference between `std::next` and `std::ranges::next`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129233