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

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: unittests/Support/IteratorTest.cpp:113
@@ +112,3 @@
+TEST(FilterIteratorTest, StdFunction) {
+  std::function<bool(int)> IsOdd = [](int a) { return a % 2 == 1; };
+  int a[] = {0, 1, 2, 3, 4, 5, 6};
----------------
Rather than adding something complex like std::function to the test - might be worth just having a simple stateful function object that exercises whatever specific behavior you're interested in.

================
Comment at: unittests/Support/IteratorTest.cpp:137
@@ +136,3 @@
+  using PointeeIterator = pointee_iterator<std::unique_ptr<int> *>;
+  const auto Range = make_filter_range(
+      make_range(PointeeIterator(a), PointeeIterator(a + 7)), IsOdd);
----------------
we don't usually put 'const' on value typed locals - I'd skip this unless there's a particular need/reason (similar feedback for all const locals)

================
Comment at: unittests/Support/IteratorTest.cpp:156
@@ +155,3 @@
+  const auto Range = make_filter_range(
+      make_range(InputIterator(a), InputIterator(a + 7)), IsOdd);
+  SmallVector<int, 3> Actual(Range.begin(), Range.end());
----------------
dblaikie wrote:
> rather than repeating '7' here (the length of the array) you can use llvm::array_lengthof, but probably better than that, just use std::end(a) (and std::begin(a) if you like symmetry). Same goes for other cases of the array length constants in this test file.
Probably use make_filter_range here and elsewhere in the tests.


https://reviews.llvm.org/D22951