[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: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:
> > 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).
> The macro is removed. and now the algorithm take the additional "tag" type. but with slightly different approach. the tag type dictates how to make the result. so the "classic tag" simply returns the output iterator and the "ranges tag" creates the `set_intersection_result`.
> 
> The benefit is to avoid writing intermediate struct (or avoid using tuple which isn't very readable). also it avoids writing the code that converts the intermediate result (temporary struct or tuple) to the final `set_intersection_result`
> 
> What do you think?
I definitely prefer this to the previous state, but I still think it's a minor optimization that isn't worth its cost in extra complexity. If these extra copies ever come up as a performance issue, we can always do the optimization later. I don't see a strong reason to avoid writing an intermediate struct -- it seems like it should be just ~6 lines of very straightforward boilerplate, mostly confined to a single internal header (but perhaps I'm missing something?).



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.intersection/ranges_set_intersection.pass.cpp:585
+
+  runAllIteratorPermutationsTests();
+
----------------
huixie90 wrote:
> var-const wrote:
> > You probably need to make this function return `bool` and add a static assertion here, similar to how `test` is invoked twice above. If there's a reason not to, please add it as a comment.
> Unfortunately that does not work because it exceeds the constexpr execution step limit, due to the large number of combination of  types of iterators (it is a 3-dimentional cartesian product)
> 
> The workaround here is instead of having one single `static_assert` that tests all the combinations, in the `runAllIteratorPermutationsTests` function, it has lots of smaller `static_assert` and each of them test 2-dimentional cartesian product which is less than the step limit.  
> And this explains the other comment you have, where I have `return true` in the second layer test function.
Thanks for explaining! Please add this rationale as a comment here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129233