[PATCH] D31171: Improve StringMap iterator support.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/unittests/ADT/StringMapTest.cpp:287-300
+TEST_F(StringMapTest, IterSetKeys) {
+  StringSet<> Set;
+  Set.insert("A");
+  Set.insert("B");
+  Set.insert("C");
+  Set.insert("D");
+
----------------
dblaikie wrote:
> Not sure this test is necessary (I mean, depending on your perspective) - since it's testing the same API in StringMap that's inherited by StringSet - there's no new/interesting code under test here, compared to the previous test.
> 
> (should StringSet's iterators actually be key iterators, rather than a 'keys()' operation on a set, which doesn't quite make sense - it only has keys, no values, so why would there be two different ranges to iterate over?)
> Not sure this test is necessary (I mean, depending on your perspective) - since it's testing the same API in StringMap that's inherited by StringSet - there's no new/interesting code under test here, compared to the previous test.

True, but the same could be set for `join_range` vs `join` in the other patch.  It doesn't matter to me either way though.

> should StringSet's iterators actually be key iterators, rather than a 'keys()' operation on a set, which doesn't quite make sense - it only has keys, no values, so why would there be two different ranges to iterate over

I could rename the function in `StringSet` by bringing `keys()` into protected scope with a `using` declaration, then make a function in `StringSet` called `values()` which simply returns the result of `keys()`.  Thoughts?


https://reviews.llvm.org/D31171