[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
Sat Feb 19 08:07:48 PST 2022
Quuxplusone added subscribers: CaseyCarter, Quuxplusone.
Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.
Partially reviewed. I cut out in the middle of `iter_move`; I assume my comment there also applies to `iter_swap`.
================
Comment at: libcxx/include/__iterator/iterator_traits.h:145
+// The `cpp17-*-iterator` exposition-only concepts are easily confused with the Cpp17*Iterator tables, so the
+// exposition-only concepts have been banished to a namespace that makes it obvious they have a niche use-case.
namespace __iterator_traits_detail {
----------------
var-const wrote:
> This is a drive-by change: I just found the original comment unclear.
Sure. FWIW, I'd just remove this comment. It kinda seems like the only point of this comment is to explain something that the comment itself says is "obvious," which means it's like //self-referentially// bad. Plus, I don't even know what it's talking about. If "the Cpp17*Iterator tables" make an appearance in libc++'s codebase, I don't know where.
Perhaps all this comment means is: "`__iterator_traits_detail::__cpp17_iterator` is not the same as `__is_cpp17_input_iterator`." But that really //is// obvious, and only serves to raise more questions, like "Wait, why isn't it?" So again, I think the solution is to delete the confusing comment.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:81
+ using reference = typename iterator_traits<_Iter>::reference;
#endif
----------------
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.
================
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*());
----------------
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->`.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:157
_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
- reverse_iterator operator+ (difference_type __n) const {return reverse_iterator(current - __n);}
+ reverse_iterator operator+ (difference_type __n) const { return reverse_iterator(current - __n); }
+
----------------
Either don't mess with the spacing at all, or fully update it as shown. (I have a slight preference for "don't mess with," but either is OK.)
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:172-178
+ _LIBCPP_HIDE_FROM_ABI friend constexpr
+ 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();
+ return ranges::iter_move(--__tmp);
+ }
----------------
clang-format strikes again. Ditto `iter_swap`.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:49
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+ static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>, "The Iterator "
+ "template argument must be a bidirectional iterator.");
----------------
var-const wrote:
> http://eel.is/c++draft/reverse.iterators#reverse.iter.requirements-1:
>
> > "The template parameter `Iterator` shall either meet the requirements of a `Cpp17BidirectionalIterator` (`[bidirectional.iterators]`) or model `bidirectional_iterator` (`[iterator.concept.bidir]`)."
>
> My understanding is that this is IFNDR, but adding a check seems like it could prevent harder-to-read errors during instantiation.
>
> There's a similar requirement for random access iterators:
>
> > "Additionally, `Iterator` shall either meet the requirements of a `Cpp17RandomAccessIterator` (`[random.access.iterators]`) or model `random_access_iterator` (`[iterator.concept.random.access]`) if the definitions of any of the members `operator+`, `operator-`, `operator+=`, `operator-=` (`[reverse.iter.nav]`), or `operator[]` (`[reverse.iter.elem]`), or the non-member operators (`[reverse.iter.cmp]`) `operator<`, `operator>`, `operator<=`, `operator>=`, `operator-`, or `operator+` (`[reverse.iter.nonmember]`) are instantiated (`[temp.inst]`)."
>
> I'm not sure that can be done with a `static_assert` -- I tried placing it into the bodies of the mentioned operators, and the compiler produces errors trying to instantiate non-existing functions before it evaluates the `static_assert`.
clang-format strikes again, in the formatting here; please fix it up somehow.
I'm ambivalent on adding this `static_assert`. It will probably //worsen// the error messages, actually, since "such-and-such operator doesn't exist when I tried to use it" is generally much easier to deal with than "this random static_assert was false instead of true." Have you actually looked at the error messages for e.g. `reverse_iterator<std::forward_list<int>::iterator>`, and //do// you think this is an improvement?
Here's the state of the art, for comparison: https://godbolt.org/z/nne1Mvzan
```
__iterator/reverse_iterator.h:120:37: error: cannot decrement value of type 'std::__forward_list_iterator<std::__forward_list_node<int, void *> *>'
reverse_iterator& operator++() {--current; return *this;}
^ ~~~~~~~
```
I predict you'll find that this static_assert is not an improvement, and decide not to keep it.
================
Comment at: libcxx/include/__iterator/reverse_iterator.h:63
+#endif
+ using iterator_category = _If<__is_cpp17_random_access_iterator<_Iter>::value,
+ random_access_iterator_tag,
----------------
var-const wrote:
> Note: this change to `iterator_category` was done previously for all language modes, even though it only applies to C++20 and later.
Right, we want to do it in all language modes, just in case someone ever tries to use some Boost contiguous_iterator tag in C++17 mode or something.
================
Comment at: libcxx/include/iterator:223
+ using iterator_category = see below; // since C++20
+ using value_type = typename iterator_traits<Iterator>::value_typTESTED e; // since C++17, until C++20
+ using value_type = iter_value_t<Iterator>; // since C++20
----------------
================
Comment at: libcxx/include/iterator:229
+ using reference = typename iterator_traits<Iterator>::reference; // until C++20
+ using reference = iter_reference_t<Iterator>; // sinceC++20
----------------
================
Comment at: libcxx/include/iterator:221
+ using iterator_concept = see below; // since C++20
+ using iterator_category = typename iterator_traits<Iterator>::iterator_category; // since C++17, until C++20
+ using iterator_category = see below; // since C++20
----------------
var-const wrote:
> Some of these changes might be a bit pedantic -- please let me know.
FWIW, I'll never care much about synopsis comments. However, it seems like you've got several (trivial?) copy-paste errors.
================
Comment at: libcxx/test/libcxx/iterators/predef.iterators/reverse.iterators/bad_template_argument.fail.cpp:22-23
+ {
+ using BadIter = std::reverse_iterator<forward_iterator<int*>>;
+ BadIter i; //expected-error-re@*:* {{static_assert failed{{.*}} "The Iterator template argument must be a bidirectional iterator."}}
+ }
----------------
I believe you wanted this to be `.verify.cpp`, not `.fail.cpp`. I think (but am not sure) that `.fail.cpp` just checks for //any// compiler failure, and doesn't actually respect `//expected-error` comments.
However, I expect this to be moot when you decide to remove the static_assert again. :)
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp:58-60
+using sized_iterator = random_access_iterator<int*>;
+static_assert( std::sized_sentinel_for<sized_iterator, sized_iterator>);
+static_assert( std::sized_sentinel_for<std::reverse_iterator<sized_iterator>, std::reverse_iterator<sized_iterator>>);
----------------
These three lines don't seem to add anything, but it would be nice to have a test for some type `B` such that `bidirectional_iterator<B> && !random_access_iterator<B> && sized_sentinel_for<B, B>`. Unfortunately I think we don't have such a type in test_iterators.h, but it might take only ~15 lines to write one here (especially since you don't have to //implement// any of its methods, just declare them).
I also think you should actually split these lines out into `reverse.iterators/sized_sentinel.compile.pass.cpp`, and add another test for a type `R` such that `essentially-random_access_iterator<R> && disable_sized_sentinel_for<R, R>`, and make sure that `requires { reverse_iterator<R>() - reverse_iterator<R>(); } == false`.
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp:22
static_assert(std::sentinel_for<I1, I1>);
- static_assert(std::sentinel_for<I1, std::reverse_iterator<float*>>);
- static_assert(!std::sized_sentinel_for<I1, std::reverse_iterator<float*>>);
----------------
var-const wrote:
> These don't work anymore because there is no `operator==` to compare `I1::base() == std::reverse_iterator<float*>::base()`. Previously, trying to compare the two would fail upon instantiation, but this check didn't trigger instantiation, so it could succeed (though arguably it shouldn't have).
>
> If you have ideas on how to replace these checks, please let me know.
This is one of these awful metaprogrammed tests. I don't even know what `I1` //is//, here, so I don't see why it should be a sentinel for `reverse_iterator<float*>` in particular. Deleting these lines seems great to me (and if you want to rewrite this whole test, that would be even greater ;))
To replicate the possible intent of these lines, you might add at the bottom of this test file
```
static_assert( std::equality_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
static_assert(!std::equality_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
static_assert( std::totally_ordered_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
static_assert(!std::totally_ordered_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
static_assert( std::three_way_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
static_assert(!std::three_way_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
```
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp:67-77
+template <class T, class = decltype(std::declval<T>() == std::declval<T>())>
+std::false_type equality_comp_SFINAEs_away_impl(int);
+
+template <class T>
+std::true_type equality_comp_SFINAEs_away_impl(...);
+
+template <class T>
----------------
Since you're in C++20-only and have concepts, please
```
template<class T> concept HasEqualTo = requires (T t) { t == t; };
template<class T> concept HasNotEqual = requires (T t) { t != t; };
template<class T> concept HasLess = requires (T t) { t < t; };
template<class T> concept HasLessEqual = requires (T t) { t <= t; };
template<class T> concept HasGreater = requires (T t) { t > t; };
template<class T> concept HasGreaterEqual = requires (T t) { t >= t; };
template<class T> concept HasSpaceship = requires (T t) { t <=> t; };
```
and then use those throughout. And you might as well test all 7 concepts for each of your types.
You know I'm not thrilled with `: IterBase`, but it seems to be saving a lot of lines of code, so I won't complain (much) this time. ;) However, doesn't this all run afoul of your `static_assert`, too? It's not legal to create the type `reverse_iterator<NoEqualityCompIter>` for at least two reasons: an iterator //must// be comparable to itself, and an iterator must also have `operator++`, which none of these do. But you needn't do anything about this yet... just wait for @CaseyCarter to tell us that it makes MSVC's implementation unhappy. ;)
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp:78-79
-int main(int, char**)
-{
- A a;
- test(&a+1, A());
+template <class T, class = decltype(&T::operator->)>
+std::false_type bad_arrow_SFINAEs_away_impl(int);
----------------
Can you find a way to use `concept`/`requires` in this file too? This one is harder because it's not C++20-and-concepts-only, but maybe with some `TEST_STD_VER` ifdefs...? I also notice the @Mordante-irking use of `_LIBCPP_HAS_NO_CONCEPTS` below; //and// the gratuitous drive-by reformatting of `main`. I think we could kill all three birds with one stone if you just split out these new tests into `arrow.sfinae.pass.cpp` (`UNSUPPORTED: c++03, c++11, c++14, c++17`; `UNSUPPORTED: libcpp-has-no-concepts`), and then don't touch `arrow.pass.cpp` at all. Does that work for you?
================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp:25-32
+template <class T>
+struct BidirectionalIteratorTraits {
+ using value_type = T;
+ using difference_type = ptrdiff_t;
+ using pointer = T*;
+ using reference = const T&;
+ using iterator_category = std::bidirectional_iterator_tag;
----------------
Since this test is C++20-only, you don't actually need the old-school Big Five. You might not even need any of them, except probably `difference_type` because you have no `operator-`. But you definitely don't need `pointer` or `reference`, and I think not even `iterator_category`.
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