[libcxx-commits] [PATCH] D105456: [libcxx][algorithms] adds `std::ranges::find`

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:61
+
+    auto const range_result = ranges::find(subrange, projectable<int>{6});
+    assert(range_result.base() == iterator_result.base());
----------------
cjdb wrote:
> zoecarver wrote:
> > I think it would reduce complexity, and increase clarity in what is actually being tested if tests that had `Proj = identity` didn't use the projectable type. 
> I'm not sure I understand what you're asking for here. Could you please rephrase?
Here you're not testing anything "special". You don't need a type that's convertible. You don't need a type that's projectable. 

By using the projectable type here, you seem to indicate that it's needed for the test. Given that its name is projectable, readers might think that you're testing a projection. But you're not. I think you should just write this test as follows:
```
  using array_t = std::array<int, 10>;
  auto range = array_t{...};
  auto subrange = std::ranges::subrange(I<array_t::iterator>(range.begin()), sentinel_wrapper(range.end()));
  auto const range_result = ranges::find(subrange, 6);
```

No need for any extra complexity. 

This makes it even more clear later that when you use the projectable type, you're testing that the projections work, specifically. 


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges::find/ranges_find.pass.cpp:69
+  {
+    auto const iterator_result = ranges::find(subrange.begin(), subrange.end(), projectable<double>{3.0});
+    assert(iterator_result.base() == range.begin() + 3);
----------------
cjdb wrote:
> zoecarver wrote:
> > This is testing that the conversion operator is invoked? I'm not sure you //need// to test that, but if you want to, I'd use a specific "convertible_to_int" type or something (or even just a "convertible<T>" type). 
> Strictly speaking, this case could be omitted, but it's documenting that `int == double` //can// return true (which I think is important in light of the test below).
If you want to keep it, let's test this without the `projectable` type. (You can either use int and double directly, or you could use a `convertible<T>` type as I suggested above, either is OK with me.)

It's not clear that a type named `projectable` is convertible. And it's not clear in the implementation of the type why it's important that there's a conversion operator. 

Here it could be much more clear what's being tested, simply by not using the projectable type. That would also make this test simpler. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456