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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/set_intersection.h:32
+#else
+    tuple< __uncvref_t<_InIter1>,
+           __uncvref_t<_Sent1>,
----------------
huixie90 wrote:
> 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 
I'd prefer to have a three-element constructor -- it is a bit of boilerplate, but I think it's vastly preferable over the downsides of using a tuple.



================
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
----------------
huixie90 wrote:
> 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?
I'd avoid using macros if at all possible. I think you could achieve a similar result by passing a template type as a tag to distinguish whether the function is invoked by the classic algorithm or the ranges algorithm (the return type would be `decltype(auto)` and the function could use `if constexpr` to return different types).

However, I think it's more trouble than it's worth. For the classic algorithm, the compiler should be able to optimize away the unused variables and their computation. In cases like this, IMO a simpler and more obviously correct implementation is higher priority than the minor performance difference (which should be optimized away in any optimized build).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129233