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

[Zachary Turner via Phabricator via llvm-commits]

zturner 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>
----------------
Can we use `std::rbegin()` and `std::rend()` instead of `C.rbegin()` and `C.rend()`?  This way it works for arrays too.


================
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());
-}
-
----------------
Can we put these two functions in the `detail` namespace?  


================
Comment at: include/llvm/ADT/STLExtras.h:237
-template <typename ContainerTy>
-auto reverse(ContainerTy &&C,
-             typename std::enable_if<has_rbegin<ContainerTy>::value>::type * =
----------------
I'd rather overload `make_reverse_iterator()` instead of `reverse()`, then have one implementation of `reverse()`


================
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());
----------------
Do we need the `enable_if`?  Won't the non-existence of `C.rbegin()` trigger SFINAE and fall back to the second overload?


https://reviews.llvm.org/D45792