[PATCH] D22951: [ADT] Add filter_iterator for filtering elements
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 12 11:08:11 PDT 2016
dblaikie added a comment.
In https://reviews.llvm.org/D22951#501451, @chandlerc wrote:
> 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"?
I'll vote for "filter" here - at least the version that takes an existing range (& perhaps that's the only version we should have - we already have utilities for making ranges out of a pair of iterators, so users with 'legacy' iterator APIs can use that to construct a range to pass to filter).
As for the name of the iterator - I'd be OK making that an unmentionable implementation detail. Most of the time its type is going to include a lambda type anyway, right - and thus be unmentionable.
================
Comment at: include/llvm/ADT/STLExtras.h:305
@@ +304,3 @@
+ friend iterator_range<filtered_iterator<WT, PT>>
+ make_filter_range(iterator_range<WT>, PT);
+};
----------------
If you like you can just define the friend inline at the friend declaration:
friend void foo() {
...
}
================
Comment at: unittests/Support/IteratorTest.cpp:161-163
@@ +160,5 @@
+ IsOdd);
+ ASSERT_EQ(3, std::distance(Range.begin(), Range.end()));
+ SmallVector<int, 3> Actual(Range.begin(), Range.end());
+ EXPECT_EQ((SmallVector<int, 3>{1, 3, 5}), Actual);
+}
----------------
The distance tests probably aren't necessary if you're testing for exact equality anyway?
https://reviews.llvm.org/D22951
More information about the llvm-commits
mailing list