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

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

Quuxplusone added inline comments.


================
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_;}
----------------
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_)))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056