[PATCH] D25534: Implement part of P0031; adding `constexpr` to `reverse_iterator`
Eric Fiselier via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 13 16:25:35 PDT 2016
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM modulo inline comments.
================
Comment at: include/iterator:95
+ template <class U> constexpr reverse_iterator(const reverse_iterator<U>& u);
+ template <class U> constexpr reverse_iterator& operator (const reverse_iterator<U>& u);
+ constexpr Iterator base() const;
----------------
Missing an `=` here.
================
Comment at: test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.ops/reverse.iter.op=/reverse_iterator.pass.cpp:53
+ constexpr std::reverse_iterator<const Derived *> it1 = std::make_reverse_iterator(p);
+ constexpr std::reverse_iterator<const Base *> it2 = (std::make_reverse_iterator(p) = it1);
+ static_assert(it2.base() == p);
----------------
This actually tests the copy assignment operator since `std::make_reverse_iterator(p)` returns `reverse_iterator<const Derived *>`.
Maybe:
```
using BaseIt = std::reverse_iterator<const Base *>;
BaseIt it2 = (BaseIt{nullptr} = it1);
```
It would also be nice if `p` was non-null so we could test that the actual assignment took place.
================
Comment at: test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.ops/reverse.iter.op=/reverse_iterator.pass.cpp:54
+ constexpr std::reverse_iterator<const Base *> it2 = (std::make_reverse_iterator(p) = it1);
+ static_assert(it2.base() == p);
+ }
----------------
The static assert is missing a message and is poorly aligned.
================
Comment at: test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.ops/reverse.iter.opref/op_arrow.pass.cpp:113
+
+ static_assert(it1->get() == gC.get(), "");
+ }
----------------
How about just:
```
constexpr const char *p = "123456789";
constexpr auto it = std::make_reverse_iterator(p+1);
static_assert(it.operator->() == p, "");
```
https://reviews.llvm.org/D25534
More information about the cfe-commits
mailing list