[PATCH] D45853: [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:298
           typename std::common_type<
-              std::forward_iterator_tag,
+              typename detail::fwd_or_bidi_tag<WrappedIteratorT>::type,
               typename std::iterator_traits<
----------------
timshen wrote:
> Would `std::bidirectional_iterator_tag` simply work?
Neat, it would've worked.

However, I did still need detail::fwd_or_bidi_tag to implement the specialized version of filter_iterator_impl you described. That's because you'd need to check if the wrapped iterator's category is derived from bidirectional_iterator_tag to make the specialization work.


================
Comment at: include/llvm/ADT/STLExtras.h:309
   struct PayloadType {
+    WrappedIteratorT Begin;
     WrappedIteratorT End;
----------------
timshen wrote:
> If WrappedIteratorT is not bidirectional, we don't need this data member. Maybe add a comment about this optimization opportunity?
As you pointed out, the Begin member isn't needed in the bidirectional case either, so I've removed it.


================
Comment at: include/llvm/ADT/STLExtras.h:324
+    assert(Payload && "Payload should be engaged when findPrevValid is called");
+    while (this->I != Payload->Begin && !Payload->Pred(*this->I))
+      BaseT::operator--();
----------------
timshen wrote:
> I think it's better to have:
>   while (!Payload->Pred(*this->I))
> 
> Notice that as it is described in http://en.cppreference.com/w/cpp/concept/BidirectionalIterator, the precondition is "a is decrementable (there exists such b that a == ++b)". Therefore, there must be an iterator value that satisfies `Payload->Pred(*this->I)`. If users wrote a bug by decrementing on begin, I'd rather keep looping and let sanitizers catch it.
I see, sounds good! That makes it possible to get rid of the Begin member.


================
Comment at: include/llvm/ADT/STLExtras.h:344
 
+  // TODO:
+  // typename std::enable_if<std::is_same<std::bidirectional_iterator_tag,
----------------
timshen wrote:
> SFINAE wouldn't work here unless you make operator--() a function template, which is an overkill.
> 
> I suggest to specialize the class on bidirectional iterator. It might require some boilerplate. For example:
>   template <typename Iter,
>                     typename Predicate,
>                     typename iter_tag = typename iterator_traits<Iter>::iterator_category>
>   class filter_iterator_impl { ... };
> 
>   template <typename Iter, typename Predicate>
>   class filter_iterator_impl<Iter, Predicate, std::bidrectional_iterator_tag> { ... };
> 
>   template <typename Iter, typename Predicate>
>   using filter_iterator = filter_iterator_impl<Iter, Predicate>;  // to hide template parameter "iter_tag".
Thanks, this worked out.


https://reviews.llvm.org/D45853