[libcxx-commits] [PATCH] D128983: [libcxx][ranges] implement `std::ranges::set_difference`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added a comment.

Thanks for addressing the feedback!



================
Comment at: libcxx/include/__algorithm/make_projected.h:50
+decltype(auto) __make_projected_comp(_Comp& __comp, _Proj1& __proj1, _Proj2& __proj2) {
+  if constexpr (same_as<_Proj1, identity> && same_as<_Proj2, identity> && !is_member_pointer_v<decay_t<_Comp>>) {
+    // Avoid creating the lambda and just use the pristine comparator -- for certain algorithms, this would enable
----------------
Question: why is this check necessary (`is_member_pointer`)?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:244
+constexpr void withAllInIter1InIter2Iterators() {
+  withAllInIter1Iterators<cpp20_input_iterator<int*>, OutIter>();
+  withAllInIter1Iterators<forward_iterator<int*>, OutIter>();
----------------
huixie90 wrote:
> var-const wrote:
> > Question: does `cpp17_input_iterator` not work? If yes, can you add a short comment indicating why it doesn't work, to make it clear it wasn't forgotten but deliberately omitted?
> The algorithm requires the input to be `std::input_iterator` which is a c++ 20 concept. cpp_17_input_iterator might or might not work, depending on whether or not it models C++20 input_iterator concept.
> 
> IMO it doesn't add too much in the test, and at the same time it couples the test to the implementation of `cpp_17_input_iterator`. Whether or not model c++ 20's input_iterator might change for that particular class. 
> 
> If we really want to test it must work, we should probably create another class `cpp17_and_cpp20_input_iterator`
> 
> What do you think?
If a type modeling the _bare minimum_ required to satisfy `cpp17_input_iterator` doesn't meet the requirements, then we can say that `cpp17_input_iterator` doesn't work. Consider a comment like "`cpp17_input_iterator` doesn't satisfy the `std::input_iterator` constraint".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128983