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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

> 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!



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:30
+
+namespace ranges = std::ranges;
+
----------------
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. 


================
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);
----------------
Nit: you could probably pull this out of these braces and into the middle ones. 


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:147
+{
+  // static_assert(check_find<cpp17_input_iterator>());
+  check_find<cpp17_input_iterator>();
----------------
Remnants of a fun debug session? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456