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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Cool, glad this is getting added!


================
Comment at: include/llvm/ADT/STLExtras.h:239
@@ -237,1 +238,3 @@
 
+/// \brief An iterator adaptor that filters the elements of given inner
+/// iterators.
----------------
No need for '\brief'.

================
Comment at: include/llvm/ADT/STLExtras.h:242-244
@@ +241,5 @@
+///
+/// Predicate should take a parameter such that predicate(*iter) returns a bool,
+/// where iter is a WrappedIteratorT object. If the predicate returns true, the
+/// element is kept.
+///
----------------
I might rephrase this as "The predicate parameter should be a callable object that accepts the wrapped iterator's reference type and returns a bool. When incrementing or decrementing the iterator, it will call the predicate on each element and skip any where it returns false."

My goal is to highlight:
- The predicate is a callable.
- We will invoke it on every element, and invoke it while incrementing or decrementing as opposed to when dereferencing.


================
Comment at: include/llvm/ADT/STLExtras.h:246-248
@@ +245,5 @@
+///
+/// Currently it only supports operator++ but not operator--, so it's an input
+/// iterator if the underlying type is input iterator; otherwise it's a forward
+/// iterator.
+///
----------------
Why not support operator--? It seems easy?

================
Comment at: include/llvm/ADT/STLExtras.h:252
@@ +251,3 @@
+///   const auto IsOdd = [](int a) { return a % 2 == 1; };
+///   filter_iterator<int *, decltype(IsOdd)> Begin(..., IsOdd);
+/// \endcode
----------------
Why not use (and add below) a make_filter_iterator? It seems like we will often want to avoid naming the type due to the predicate.

================
Comment at: include/llvm/ADT/STLExtras.h:269-273
@@ +268,7 @@
+      filter_iterator<WrappedIteratorT, Predicate>, WrappedIteratorT,
+      typename std::conditional<
+          std::is_same<std::input_iterator_tag,
+                       typename std::iterator_traits<
+                           WrappedIteratorT>::iterator_category>::value,
+          std::input_iterator_tag, std::forward_iterator_tag>::type,
+      typename std::iterator_traits<WrappedIteratorT>::value_type,
----------------
Instead, of the std::conditional, use std::common_type:

  typename std::common_type<std::forward_iterator_tag,
                            typename std::iterator_traits<
                                WrappedIteratorT>::iterator_category>::type

This will use the derived to base conversion to select the best tag type possible, and as an advantage will fail to compile with an output iterator.

If you add operator--, then std::forward_iterator_tag above becomes std::bidirectional_iterator_tag. And you will *really* want to use common_type because the conditional gets crazy.

================
Comment at: include/llvm/ADT/STLExtras.h:274-277
@@ +273,6 @@
+          std::input_iterator_tag, std::forward_iterator_tag>::type,
+      typename std::iterator_traits<WrappedIteratorT>::value_type,
+      typename std::iterator_traits<WrappedIteratorT>::difference_type,
+      typename std::iterator_traits<WrappedIteratorT>::pointer,
+      typename std::iterator_traits<WrappedIteratorT>::reference>;
+
----------------
You don't need these four lines. The adaptor base will do this for you.

================
Comment at: include/llvm/ADT/STLExtras.h:302-309
@@ +301,10 @@
+
+template <typename WrappedIteratorT, typename Predicate>
+iterator_range<filter_iterator<WrappedIteratorT, Predicate>>
+make_filter_range(iterator_range<WrappedIteratorT> Range,
+                  const Predicate &Pred) {
+  using IterT = filter_iterator<WrappedIteratorT, Predicate>;
+  return make_range(IterT(Range.begin(), Range.end(), Pred),
+                    IterT(Range.end(), Range.end(), Pred));
+}
+
----------------
This needs its own doxygen comment.

================
Comment at: unittests/Support/IteratorTest.cpp:104
@@ +103,3 @@
+  int a[] = {0, 1, 2, 3, 4, 5, 6};
+  const auto Range = make_filter_range(make_range(a, a + 7), IsOdd);
+  ASSERT_EQ(3, std::distance(Range.begin(), Range.end()));
----------------
Please test other forms of callables. std::function, function pointer at least.

Also test an input iterator and combining filter with some of our other adaptors like a filtered pointee iterator.


https://reviews.llvm.org/D22951