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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/transform_view.h:73-74
+
+  constexpr __iterator<false> begin()
+    noexcept(noexcept(ranges::begin(__base_)))
+  {
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > Teeechnically... it's impossible for humans to write noexcept-clauses. ;) Could you test locally with an iterator type like this, and report back if it prints `OK!`?
> > > https://godbolt.org/z/fvxTo4n1v
> > Yep, it prints `OK!`. Want me to add a test like this? I think it should be covered by the noexcept tests, but maybe wouldn't hurt. 
> Aw, geez... Try again with this version, please. https://godbolt.org/z/v16bds7o1
> Did I mention it's impossible for humans to write noexcept-clauses? :D  (I accidentally left the `noexcept` off of my `Foo::begin` method.)
OK, it still prints `OK!` :P


================
Comment at: libcxx/include/__ranges/transform_view.h:98
+  constexpr __sentinel<true> end()
+    const noexcept(noexcept(ranges::end(__base_)))
+    requires range<const _View> &&
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > This `const` is on the wrong line; ditto on line 79 above; and then please rearrange these getters into any reasonable order (e.g. `begin`, `begin const`, `end`, `end const`). Right now they're in the order `begin const, begin, end, end const`, which combined with the lack of trailing `const` keyword, confused the heck out of me.
> > Yes, it's confusing. Which is worse, how it is now, or having it be in a different order from the standard wording? 
> I'm not asking for different order (I think?). Just to move the `const` up:
> ```
>   constexpr __sentinel<true> end() const
>     noexcept(noexcept(ranges::end(__base_)))
>     requires range<const _View> &&
> ```
> I'm not asking for different order (I think?)

See above

> and then please rearrange these getters into any reasonable order (e.g. begin, begin const, end, end const). Right now they're in the order begin const, begin, end, end const, which combined with the lack of trailing const keyword, confused the heck out of me.

Anyway...

> Just to move the const up:

Will do.


================
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:
> zoecarver wrote:
> > 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. 
> For testing purposes, I recommend using `int*` as your "contiguous, nothrow iterator" and `contiguous_iterator<int*>` as your "contiguous, non-nothrow iterator." Sure, the former is not only nothrow but //trivial//; but as I think you yourself have pointed out on some review a few months ago, it's not possible for us to provide //all// of the combinatorial explosion of undreamt-of possibilities here.
> 
> (Tangent: We can pretend to do so via templates, but that's just adding another combinatorial dimension: we then miss out on the "specifically non-template, etc. etc. iterator." Which makes a difference in Ranges!, because certain things (I'm thinking of `pointer_traits`) behave differently depending on whether the iterator type is a template specialization or not.
> 
> - I don't yet see a concrete reason to provide either a "specifically random-access, specifically nothrow" iterator (as you propose here) nor a "random-access, specifically non-nothrow" iterator (as we have now in trunk).
> - I don't yet see a concrete reason to provide either a "specifically bidirectional, specifically nothrow" iterator (as you //don't// propose here) nor a "bidirectional, specifically non-nothrow" iterator (as we have now in trunk). Etc.
> 
> Alternatively, could your noexceptness test make use of `int*` (nothrow) and `ThrowingIterator` (throwing)?  I see `ThrowingIterator` being used in `test/std/ranges/range.access/range.access.begin/begin.pass.cpp` already.
> 
> Anyway, I'm still not philosophically //opposed// to nothrow-ing our test iterators, but it should be done consistently and in a separate PR, and it should have some rationale in the commit message about why you're noexcept-ing `operator*` but not `operator++` (etc. etc.)  What I //am// philosophically opposed to, is making ad-hoc changes to the signatures of specific test iterators for the subtle benefit of //one particular test// you want to write. After all, someone else might have already made them non-noexcept for the subtle benefit of some //other// test, which you would now be quietly nerfing.
> 
> Oh, and if you do pursue the separate-PR-consistently idea, I think it should be `noexcept(noexcept(reference(*it_)))`.
> For testing purposes, I recommend using int* as your "contiguous, nothrow iterator" and contiguous_iterator<int*> as your "contiguous, non-nothrow iterator." 

Done :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056