[libcxx-commits] [PATCH] D103056: [libcxx][ranges] Add `ranges::transform_view`.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 2 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:30
+
+// TODO: add noexcept tests
+
----------------
These have been added. Remove the TODO.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:39
+auto withRandom(R range, Fn func = Fn()) {
+  return std::ranges::transform_view(range, std::bind_front(func, std::rand()));
+}
----------------
Quuxplusone wrote:
> I don't understand what's going on here with `std::bind_front`, nor the defaulted function argument `func = Fn()`, but surely our unit tests should not depend on the behavior of `rand()`.
> Please switch to e.g. `[](int x) { return x + 1; }` and remove the `bind` and default-function-argument stuff.
> 
The tests in this file are not testing any one thing in particular. They're meant to be a few general use cases that we might see in the wild. I suppose you're right about random, though. I'll find another function. 


================
Comment at: libcxx/test/support/test_iterators.h:234
 
-    TEST_CONSTEXPR_CXX14 reference operator*() const {return *it_;}
+    TEST_CONSTEXPR_CXX14 reference operator*() const noexcept {return *it_;}
     TEST_CONSTEXPR_CXX14 pointer operator->() const {return it_;}
----------------
Quuxplusone wrote:
> This shouldn't be necessary.  Not necessarily a //bad// idea, but unnecessary.
> 
> If we're going to `noexcept`-qualify our test iterators' `operator*`, `operator->`, and `operator[]`, that should be done consistently across //all// of the test iterator types, and it should be done in a separate PR so we can see what effect it has (if any) on the existing test suite.
Why shouldn't this be necessary? We need some iterators with noexcept methods to be able to test the noexceptness of transform_view. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056