[libcxx-commits] [PATCH] D120180: [libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Mar 7 21:04:29 PST 2022
Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.
I'm inclined to green-light this at this point. I haven't given it the fine-tooth-comb treatment this time, but it's been through enough rounds of review and I assume nothing that we covered earlier has regressed lately. However:
- CI is red for apparently multiple reasons; please make sure it becomes green.
- I'd like a second libc++ reviewer to take a look too.
Happy to take another look if anything major comes up in the second person's review.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:179-180
+ iter_rvalue_reference_t<_Iter> iter_move(const reverse_iterator& __i)
+ noexcept(is_nothrow_copy_constructible_v<_Iter> &&
+ noexcept(ranges::iter_move(--declval<_Iter&>()))) {
+ auto __tmp = __i.base();
----------------
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:188-190
+ noexcept(is_nothrow_copy_constructible_v<_Iter> &&
+ is_nothrow_copy_constructible_v<_Iter2> &&
+ noexcept(ranges::iter_swap(--declval<_Iter&>(), --declval<_Iter2&>()))) {
----------------
Btw, the difference between
```
noexcept(ranges::iter_swap(--declval<_Iter&>(), --declval<_Iter2&>()))
```
and
```
noexcept(ranges::iter_swap(--__x, --__y))
```
is subtle. Do you have a test that will fail if someone makes that substitution? (If not, please add one.)
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:81
+ using reference = typename iterator_traits<_Iter>::reference;
#endif
----------------
var-const wrote:
> Quuxplusone wrote:
> > Please group these blocks together; see what I did in https://reviews.llvm.org/D117656#change-18S54d7VLfJo
> > I also recommend getting rid of the cute space-alignment formatting as long as you're in the area.
> PTAL -- is this what you had in mind?
Close. I'd definitely move `iterator_concept` down to coalesce those two `_LIBCPP_STD_VER > 17` blocks together. I'd also either duplicate the `iterator_category` and `pointer` lines, or move them up next to `iterator_type`, to keep the non-ifdef'ed stuff in a block together.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:139-146
+ pointer operator->() const
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+ requires is_pointer_v<_Iter>
+ || requires(const _Iter i) { i.operator->(); }
+#endif
+ {
+ return _VSTD::addressof(operator*());
----------------
var-const wrote:
> Quuxplusone wrote:
> > var-const wrote:
> > > Quuxplusone wrote:
> > > > I think what I'd do here is
> > > > ```
> > > > #if _LIBCPP_STD_VER > 17
> > > > pointer operator->() const
> > > > #if !defined(_LIBCPP_HAS_NO_CONCEPTS)
> > > > requires is_pointer_v<_Iter> || requires(const _Iter i) { i.operator->(); }
> > > > #endif // !defined(_LIBCPP_HAS_NO_CONCEPTS)
> > > > {
> > > > _Iter __tmp = current;
> > > > --__tmp;
> > > > if constexpr (is_pointer_v<_Iter>) {
> > > > return __tmp;
> > > > } else {
> > > > return _VSTD::move(__tmp).operator->();
> > > > }
> > > > }
> > > > #else
> > > > pointer operator->() const {
> > > > return _VSTD::addressof(operator*());
> > > > }
> > > > #endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
> > > > ```
> > > > Drive-by removed the cute spacing in `pointer operator->`.
> > > Done (with very slight modification).
> > >
> > > Do you think it's worth thinking whether it's possible to test for the change in the effects of this function in C++20?
> > Yes, you certainly could write a test for it, so sure, let's do it!
> > ```
> > struct It {
> > [...all the usual members...]
> > int& operator*() const; // not defined
> > int *operator->() const&; // not defined
> > int *operator->() && { return nullptr; }
> > };
> > std::reverse_iterator<It> r;
> > assert(r.operator->() == nullptr);
> > ```
> Done, thanks! (I've had to provide definitions for some of the unimportant functions -- otherwise I'm getting errors like
> ```
> function 'main(int, char **)::It::operator*' has internal linkage but is not defined [-Werror,-Wundefined-internal]
> ```
> ).
I finally notice your slight modification, and I'm not thrilled by it. Let's not break apart
```
_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
pointer operator->() const
```
from the function's curly-braced body just to save two lines of duplication. We generally try (and everyone should generally try ;)) to put preprocessor conditionals only around "sensible" units of code — like, (an extreme example,) prefer
```
#if X
return x+1;
#else
return x;
#endif
```
over
```
return x
#if X
+1
#endif
;
```
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:39
#endif
+
return 0;
----------------
This is the only diff in this test file; consider reverting it.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/sized_sentinel.compile.pass.cpp:25
+static_assert(!std::sized_sentinel_for<std::reverse_iterator<unsized_it>, std::reverse_iterator<unsized_it>>);
+static_assert(!requires { std::reverse_iterator<unsized_it>() - std::reverse_iterator<unsized_it>(); });
----------------
I don't know if they're //supposed// to work in C++20, but in practice, compilers don't like non-dependent `requires` clauses. https://reviews.llvm.org/harbormaster/unit/view/2833598/ You'll have to rewrite this kind of thing as
```
template<class T> concept HasMinus = requires (T t) { t - t; };
static_assert(!HasMinus<std::reverse_iterator<unsized_it>>);
```
which is honestly easier on the eyes anyway. :)
Circa line 20 above, you should also assert that `HasMinus<std::reverse_iterator<sized_it>>` (even though I guess it's obvious... well, just for symmetry, let's assert it anyway).
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp:85-94
+namespace std {
+template <>
+struct iterator_traits<BarIter> {
+ using difference_type = char;
+ using value_type = char;
+ using pointer = char*;
+ using reference = char&;
----------------
var-const wrote:
> Quuxplusone wrote:
> > Is the purpose of this test just to make sure that `reverse_iterator<BarIter>` takes its `difference_type` from `iterator_traits<BarIter>::difference_type` and not from `BarIter::difference_type` directly? ...But wait, `BarIter::difference_type` doesn't exist! And it wouldn't exist for an iterator type like `int*`, either. So I actually don't think this test is adding any coverage, and therefore would prefer to remove it. Unless I'm missing something.
> >
> > Which I must be, because how on earth do we end up with `is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>` on line 112??
> > Is the purpose of this test just to make sure that `reverse_iterator<BarIter>` takes its `difference_type` from `iterator_traits<BarIter>::difference_type`..?
>
> `BarIter` is only for testing `reference` (to make sure it's implemented correctly both pre- and post-C++20). `difference_type` is tested via `FooIter`.
>
> > how on earth do we end up with `is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>` on line 112?
>
> Pre-C++20, `reference` would be defined as `iterator_traits<I>::reference`, so it's taken from the specialization of `iterator_traits`. In C++20+, `reference` is `iter_reference_t`, which is defined as `decltype(*declval<T&>());`. So the type is taken from the return type of `BarIter::operator*()` (see line `82`).
Btw, I believe my confusion at first was due mainly to thinking `FooIter` and `BarIter` were the same type (i.e. I misread `Bar` as `Foo` or vice versa). I see where `bool` comes from, now.
It strikes me that `FooIter` is just plain old bidirectional_iterator — er, kinda sorta — whenever it is //not// true that `TEST_STD_VER > 17`. So really, `FooIter` is a "C++20-only test case"; you dutifully assert things about it when `<= 17`, but those things aren't really interesting things, because half of the type is missing. I think the entire definition of `FooIter` should be guarded by `TEST_STD_VER > 17`.
As I type that comment (specifically the "kinda sorta" part), I recall why inheriting from test iterators is a bad idea: you'll find that `std::bidirectional_iterator<FooIter> == false`, am I right? In fact `FooIter` isn't any kind of iterator in C++20, right? This doesn't render the test UB, but it does mean that you're not actually saving too much boilerplate here — you can //omit// all the iterator boilerplate! I bet you don't need much more than
```
#if TEST_STD_VER > 17
struct FooIter {
using value_type = void*;
using difference_type = void*;
};
template <>
struct std::indirectly_readable_traits<FooIter> {
using value_type = int;
};
template <>
struct std::incrementable_traits<FooIter> {
using difference_type = char;
};
static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::value_type, int>);
static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::difference_type, char>);
#endif // TEST_STD_VER > 17
```
The `BarIter` test should remain enabled for all C++ versions, i.e. something like
```
struct BarIter {
bool& operator*() const;
};
template <>
struct std::iterator_traits<BarIter> {
using difference_type = char;
using value_type = char;
using pointer = char*;
using reference = char&;
using iterator_category = std::bidirectional_iterator_tag;
};
#if TEST_STD_VER > 17
static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>);
#else
static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, char&>);
#endif
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120180/new/
https://reviews.llvm.org/D120180
More information about the libcxx-commits
mailing list