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

Chandler Carruth via llvm-commits llvm-commits at lists.llvm.org
Sat Jul 30 02:30:58 PDT 2016


chandlerc added a comment.

One high level question that I'd love others' thoughts on: should this be spelled "filter_iterator" and "make_filter_range" or "filtered_iterator" and "make_filtered_range". For a type, I personally prefer "filtered_iterator". For the factory function, I wonder if we want something more like "reverse", perhaps "filter"?


================
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>;
+
----------------
timshen wrote:
> chandlerc wrote:
> > You don't need these four lines. The adaptor base will do this for you.
> The adaptor will do PointerT = T* and ReferenceT = T& for me, but I want to forward WrappedIteratorT's pointer and reference type in filter_iterator (because the filtered elements have the same type as the original ones).
If we want to change this it should be changed in iterator_adaptor_base, not specialized here IMO.

And if you're going to fix that, it should be in a separate patch from the one adding the new iterator.

================
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; });
----------------
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

================
Comment at: include/llvm/ADT/STLExtras.h:252
@@ +251,3 @@
+/// \endcode
+template <typename WrappedIteratorT, typename Predicate>
+class filter_iterator
----------------
I think this should be PredicateT here (at least for consistency).

================
Comment at: include/llvm/ADT/STLExtras.h:284-287
@@ +283,6 @@
+public:
+  filter_iterator(WrappedIteratorT Begin, WrappedIteratorT End, Predicate Pred)
+      : BaseT(Begin), End(End), Pred(Pred) {
+    findNextValid();
+  }
+
----------------
I think you should make the constructor(s) for this iterator private and add some comments explaining that because even the begin iterator tracks the end, we need to construct these iterators as a range with begin and end anyways.

Then you can befriend the make_... function below.


Also, I think you should have two constructors, one that is exactly as your current, and one that accepts just one wrapped iterator, the end, and leaves the predicate uninitialized.

With that, you'll be able to avoid unnecessary copying of the predicate (see below) and we shouldn't ever be using the predicate on the end iterator based on my suggestion above.

================
Comment at: include/llvm/ADT/STLExtras.h:285
@@ +284,3 @@
+  filter_iterator(WrappedIteratorT Begin, WrappedIteratorT End, Predicate Pred)
+      : BaseT(Begin), End(End), Pred(Pred) {
+    findNextValid();
----------------
You should move all of these.

================
Comment at: include/llvm/ADT/STLExtras.h:311
@@ +310,3 @@
+
+/// \brief Convenience function that takes a range of elements and a predicate,
+/// and return a new filter_iterator range.
----------------
No need for '\brief' here either. =]

================
Comment at: include/llvm/ADT/STLExtras.h:313
@@ +312,3 @@
+/// and return a new filter_iterator range.
+template <typename WrappedIteratorT, typename Predicate>
+iterator_range<filter_iterator<WrappedIteratorT, Predicate>>
----------------
As above, this should be PredicateT.

================
Comment at: include/llvm/ADT/STLExtras.h:316
@@ +315,3 @@
+make_filter_range(iterator_range<WrappedIteratorT> Range,
+                  const Predicate &Pred) {
+  using IterT = filter_iterator<WrappedIteratorT, Predicate>;
----------------
I think you want to accept the predicate by value and move it as we should be modeling "consume" semantics here. See above for avoiding the copy.

And a by-value parameter for an empty struct is more efficient than any kind of reference.

================
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),
----------------
"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>>


https://reviews.llvm.org/D22951





More information about the llvm-commits mailing list