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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Jul 16 13:49:33 PDT 2021


ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I'll say it upfront, I dislike that we're adding these nodiscard extensions. But we've done it for the non-ranges algorithms, so it's going to be difficult to vouch for not adding the ranges ones.

IMO we should either make them nodiscard unconditionally, or not at all. Adding a knob that very few people will use is not a great bang-for-the-buck.



================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_ranges_extensions.pass.cpp:10
+
+// Test that entities declared [[nodiscard]] as at extension by libc++, are
+// only actually declared such when _LIBCPP_ENABLE_NODISCARD is specified.
----------------
`as an`

Also, the comment is the same as the one in `nodiscard_ranges_extensions.verify.cpp`, but they are testing opposite things. This one could say something like:

```
// Test that entities in <ranges> are not declared [[nodiscard]] as a libc++ extension
// if the user doesn't request it explicitly.
```



================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_ranges_extensions.pass.cpp:33
+
+  ranges::find(ranges::begin(arr), ranges::end(arr), 1);
+  ranges::find(arr, 1);
----------------
You can make this test `.verify.pass.cpp`, and then use `// expected-no-diagnostics` to make the intent clearer.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
----------------
Throughout these tests, we never call `ranges::find` with another type than a `complexity_iterator`. Sure, we pass it a `complexity_iterator` wrapping various iterator archetypes, but the actual type passed to `ranges::find` is still always the `complexity_iterator`. We need to pass the actual archetypes too in order to really check that `ranges::find` works with those.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:15
+
+// ranges::find
+
----------------
Can you spell out the prototype(s) you're testing, and make it one per file?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find.pass.cpp:130
+      auto input = ranges::subrange(complexity_iterator(I(range.begin())), sentinel_wrapper(range.end()));
+      auto const result = ranges::find(input.begin(), input.end(), value, successor);
+      assert(result.base().base() == expected_result);
----------------
Can you spell out these types explicitly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105456



More information about the libcxx-commits mailing list