[PATCH] D45792: [ADT] Teach reverse() about filter_iterator ranges

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: include/llvm/ADT/STLExtras.h:235
-// Returns an iterator_range over the given container which iterates in reverse.
-// Note that the container must have rbegin()/rend() methods for this to work.
-template <typename ContainerTy>
----------------
zturner wrote:
> Can we use `std::rbegin()` and `std::rend()` instead of `C.rbegin()` and `C.rend()`?  This way it works for arrays too.
I think this overload already works with arrays. Do you have an example of a case that's not supported?


================
Comment at: include/llvm/ADT/STLExtras.h:236-247
-template <typename ContainerTy>
-auto reverse(ContainerTy &&C,
-             typename std::enable_if<has_rbegin<ContainerTy>::value>::type * =
-                 nullptr) -> decltype(make_range(C.rbegin(), C.rend())) {
-  return make_range(C.rbegin(), C.rend());
-}
-
----------------
zturner wrote:
> Can we put these two functions in the `detail` namespace?  
I can do that in a follow-up. Aside from make_reverse_iterator, which other function did you have in mind?


================
Comment at: include/llvm/ADT/STLExtras.h:237
-template <typename ContainerTy>
-auto reverse(ContainerTy &&C,
-             typename std::enable_if<has_rbegin<ContainerTy>::value>::type * =
----------------
zturner wrote:
> I'd rather overload `make_reverse_iterator()` instead of `reverse()`, then have one implementation of `reverse()`
This doesn't seem to be possible. A filter_iterator pointing to the end of the sequence can't meaningfully be reversed, because the Payload would be missing.


================
Comment at: include/llvm/ADT/STLExtras.h:238-239
-auto reverse(ContainerTy &&C,
-             typename std::enable_if<has_rbegin<ContainerTy>::value>::type * =
-                 nullptr) -> decltype(make_range(C.rbegin(), C.rend())) {
-  return make_range(C.rbegin(), C.rend());
----------------
zturner wrote:
> Do we need the `enable_if`?  Won't the non-existence of `C.rbegin()` trigger SFINAE and fall back to the second overload?
Nope, nice catch, we don't need this enable_if. I'll get rid of it.


https://reviews.llvm.org/D45792