[PATCH] D47942: [SmallSet] Add SmallSetIterator.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/ADT/SmallSet.h:30
+/// delegating to the underlying SmallVector or Set iterators.
+template <typename T, unsigned N, typename C> class SmallSetIterator {
+private:
----------------
Any chance using iterator_facade (in include/llvm/ADT/iterator.h) would help simplify this implementation?


================
Comment at: include/llvm/ADT/SmallSet.h:35
+
+  bool isSmall;
+  /// Iterators to the parts of the SmallSet containing the data. They are set
----------------
Might make sense to put isSmall after the union for packing reasons (eg: if the iterators are 24 bits long and require 32 bit alignment - the existing ordering would create a 64 bit total size, but swapping it around would allow a 32 bit size, I think?)?


================
Comment at: unittests/ADT/SmallSetTest.cpp:75-87
+TEST(SmallSetTest, Iterator) {
+  SmallSet<int, 4> s1;
+
+  for (int i = 0; i < 8; i++)
+    s1.insert(i);
+
+  std::vector<int> V(s1.begin(), s1.end());
----------------
Does this test both the small and non-small cases?


https://reviews.llvm.org/D47942