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

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: include/llvm/ADT/STLExtras.h:311
@@ +310,3 @@
+iterator_range<filter_iterator<WrappedIteratorT, PredicateT>>
+make_filter_range(iterator_range<WrappedIteratorT> Range, PredicateT Pred) {
+  using FilterIteratorT = filter_iterator<WrappedIteratorT, PredicateT>;
----------------
This seems to be the wrong API - the function should accept any range, not just an iterator_range. (ie: you should be able to pass a std::vector to this function directly (or a std::set, SmallVector, DenseMap, etc))

So it should probably take the range as totally generic type T, as an rvalue reference/perfect forwarding.

This then gets into the issue of temporaries - which came up recently in the "zip" thread. It'd be good to resolve this aspect of teh design for both these utilities, before they go in.

(or, if you're in a hurry, just put some big comment saying this will do bad things with temporaries+range-for, for now)

================
Comment at: unittests/Support/IteratorTest.cpp:182
@@ +181,3 @@
+  auto Range = make_filter_range(
+      make_range(InputIterator(std::begin(A)), InputIterator(std::end(A))),
+      IsOdd);
----------------
Should just be able to pass "A" here, rather than "make_range(InputIterator(std::begin(A)), InputIterator(std::end(A)))"

Similarly in other test cases. I suppose the Composition test isn't able to do that (though the pointee iterator should have a wrapper utility that works as a range adapter) - but I'm not sure how useful/important that test is anyway. The iterators should be orthogonal, so testing combinations seems like it'd create an unbounded test matrix.


https://reviews.llvm.org/D22951