[PATCH] D22951: [ADT] Add filter_iterator for filtering elements

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: include/llvm/ADT/STLExtras.h:246-248
@@ +245,5 @@
+///
+/// \code
+///   int A[] = { 1, 2, 3, 4 };
+///   const auto R = make_filter_range(A, A + 4,
+///                                    [](int A) { return A % 2 == 1; });
----------------
chandlerc wrote:
> I think I was very wrong to suggest this. IT's actually really hard to support being bi-directional, because now your iterator needs to retain *both* begin and end in addition to the current position so that it can stop backing up when necessary. The code below, if you back up from end and filter out begin, will walk completely past the start of the wrapped iterator. Oops.
> 
> It also forces you to copy the predicate into the begin and the end in order to be able to move the end iterator.
> 
> All of this complexity doesn't seem worth it when the user can always reverse the wrapped iterator rather than reversing this iterator. Go back to your original (simpler) design and make it a forward iterator. =D
> ...so that it can stop backing up when necessary.

I don't think so, since when a user calls operator--() and our wrapped iterator goes past the original begin, the user deserve the undefined behavior, right? It's like in general calling operator--() on begin().

> It also forces you to copy the predicate into the begin and the end in order to be able to move the end iterator.

Great point. Saving one predicate copy sounds awesome. I'll change it back to a forward iterator.

================
Comment at: include/llvm/ADT/STLExtras.h:274-277
@@ +273,6 @@
+      typename std::iterator_traits<WrappedIteratorT>::reference>;
+
+  WrappedIteratorT End;
+  Predicate Pred;
+
+  void findNextValid() {
----------------
Changing iterator_adaptor_base is weird, because forwarding the WrappedIteratorT's typedefs only makes sense in filter_iterator. In filter_iterator, the wrapped iterator has the same element as the filter_iterator has, while in the general iterator_adaptor_base it's not true.

Notice that filter_iterator doesn't require `typename T` parameter as iterator_adaptor_base does.

================
Comment at: include/llvm/ADT/STLExtras.h:317
@@ +316,3 @@
+                  const Predicate &Pred) {
+  using IterT = filter_iterator<WrappedIteratorT, Predicate>;
+  return make_range(IterT(Range.begin(), Range.end(), Pred),
----------------
chandlerc wrote:
> "IterT" is pretty ambiguous in the context including WrappedIteratorT... Also, I'd create the alias in the template parameter list so you can use it in the return type:
> 
>   template <typename WrappedIteratorT, typename PredicateT>
>             typename FilterIteratorT =
>                 filter_iterator<WrappedIteratorT, PredicateT>>
It's really hard to let a class friend declaration cope with default template argument. I will leave the local type alias alone, with a better name FilterIteratorT.


https://reviews.llvm.org/D22951