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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/set_intersection.h:32
+#else
+    tuple< __uncvref_t<_InIter1>,
+           __uncvref_t<_Sent1>,
----------------
var-const wrote:
> 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.
That was my first attempt. it turns out that in c++03 , initialising an aggregate is so hard. I can’t just ` return myStrucr{a,b};` I have to declare a variable and then return it. And by declaring a variable , it might be copied (NRVO is not guaranteed ). So I have to define a proper constructor for it to work for c++03

That is too much effort and I gave up and use tuple

But I still like the idea and struct with named members. what about only use struct for c++11 onward and for 03 just guard with macro 


================
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
----------------
var-const wrote:
> 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`.
I did consider that. But for claasic algorithm, there is no need to call next since the iterators are discarded. std::next can be a linear complexity operation and it is wasteful to compute it when the result is not needed at all. 

That is why I used this approach so that only ranges overload computes the next

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129233