[libcxx-commits] [PATCH] D102028: [libcxx][ranges] Implement views::all.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

Sure, I don't need to be red anymore.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp:23
+
+template<bool IsNoexcept = true>
+struct ViewImpl : std::ranges::view_base {
----------------
Please remove the default template parameter here: just `template<bool IsNoexcept>`. Otherwise, you run the risk of writing `ViewImpl` (CTAD) when you meant `ViewImpl<false>`. Be explicit.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp:37
+
+static_assert(std::ranges::view<View>);
+
----------------
Let's static_assert here on both specializations, unless there's a reason not to.
```
static_assert(std::ranges::view<ViewImpl<true>>);
static_assert(std::ranges::view<ViewImpl<false>>);
```
In fact, you might consider renaming `ViewImpl` to just `View`, merely to shorten the line lengths throughout. Ditto for `CopyableViewImpl`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp:94-95
+    ASSERT_SAME_TYPE(decltype(std::views::all(View())), View);
+    static_assert(noexcept(std::views::all(View())));
+    static_assert(!noexcept(std::views::all(ViewImpl<false>())));
+
----------------
Not a blocker, but I will note for @ldionne's benefit that we have recently seen (in D103769) that merely testing top-level noexceptness is not sufficient to catch implementation bugs. What we actually want to test here is that //if the foo operation throws an exception, that exception is correctly propagated all the way up to the top level.// It's not sufficient to just test that the top level is not //marked// noexcept, because the implementor may have accidentally marked one of the intermediate levels noexcept. We need runtime tests to actually verify that the "exception pipe" from low level to top level is unbroken.

(They //should not// be marking intermediate levels noexcept, because that's often harmful and never helpful. But empirically I observe them doing it anyway.)
https://quuxplusone.github.io/blog/2018/06/12/attribute-noexcept-verify/
https://stackoverflow.com/questions/66459162/where-do-standard-library-or-compilers-leverage-noexcept-move-semantics-other-t


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102028