[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:
> 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.
> 
I used a slightly different approach now. It is combined with the other suggestion of using a "Tag" type. The "Tag" type specify what the return type should be.
So with that, I can avoid using `tuple` or any temporary struct at all. The "ClassicOperation" can specify the return type is just the iterator and the "RangeOperation" specifies that the return type is the `set_intersection_result` directly, without the need of creating any intermediate data type. 


================
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:
> 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?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.intersection/ranges_set_intersection.pass.cpp:273
+  withAllPermutationsOfInIter1<contiguous_iterator<int*>, OutIter>();
+  return true;
+}
----------------
var-const wrote:
> Unused?
it is used. see the function body of `runAllIteratorPermutationsTests` contains `static_assert`


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.intersection/ranges_set_intersection.pass.cpp:585
+
+  runAllIteratorPermutationsTests();
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129233