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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D105456#2871478 <https://reviews.llvm.org/D105456#2871478>, @zoecarver wrote:

>> Just a note on the two comments above. I understand how they might come across as pedantic, or too picky, but I think it's important that we get this right. If we develop a good model here, it will mean we have a good model for the dozens of algorithms to come. That's why I think this patch should have a high bar (which is not to say that I wouldn't have made those two comments on any other patch).
>
> Somehow phab got stuck on the old patch so I didn't see the new changes. Sorry for both adding churn and making comments about things that are now resolved.
>
> This LGTM now, but I want to check out your other algorithms patches so I can see how you're testing things over there. I don't have time to do that //right now// but I figured I'd get this comment in ASAP so you see that I don't actually have a problem with these tests anymore :)
>
> Thanks again for working on this!

It's a bit confusing, but the only other one that you //need// to check out is D105795 <https://reviews.llvm.org/D105795> (all the rest are the same as this patch). Please report your feedback to D105791 <https://reviews.llvm.org/D105791>, since that's where the idea for `test_algorithm` is proposed.



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:30
+
+namespace ranges = std::ranges;
+
----------------
zoecarver wrote:
> I would like us to find some consistency with this. As I said before, I don't really care what we actually do. But I'd like to know what I should do and what you should do so we can stop spending time thinking about it. If this patch lands, we'll do three different things for apparently no reason across our test suite. 
I've defaulted to my original alias because `stdr` is too easily mistyped or misread as `std`. I can go and clean the rest up in an NFC.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:44
+    {
+      auto input = ranges::subrange(complexity_iterator(I(range.begin())), sentinel_wrapper(range.end()));
+      auto const result = ranges::find(input.begin(), input.end(), value);
----------------
zoecarver wrote:
> Nit: you could probably pull this out of these braces and into the middle ones. 
Negative: input ranges mean we need to recreate the range each time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456