<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Mar 21, 2017 at 10:29 AM Zachary Turner via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">zturner added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: llvm/unittests/ADT/StringMapTest.cpp:287-300<br class="gmail_msg">
+TEST_F(StringMapTest, IterSetKeys) {<br class="gmail_msg">
+  StringSet<> Set;<br class="gmail_msg">
+  Set.insert("A");<br class="gmail_msg">
+  Set.insert("B");<br class="gmail_msg">
+  Set.insert("C");<br class="gmail_msg">
+  Set.insert("D");<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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.<br class="gmail_msg">
><br class="gmail_msg">
> (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?)<br class="gmail_msg">
> 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.<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br>The difference there is, at least to me, that there's actual code for join_range - its absence would cause a test to fail. So at least a syntactic test would be nice. Semantics - yeah, that gets fuzzy. Is 'join' an implementation detail of join_range (so all the 'join' tests should be duplicated for 'join_range') or is the unit of 'join_range' really only to call 'join'. In which case, ideally, a mock test would demonstrate that join_range calls join & not test anything else - but without mocking/injection, we settle for testing one or two cases and assume the rest are all passing through fine.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
> 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<br class="gmail_msg">
<br class="gmail_msg">
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?<br class="gmail_msg"></blockquote><div><br>I'm more wondering if StringSet's begin/end should return the key iterators directly. It seems strange to me to have two ranges (the StringSet's own begin/end, and some named accessor with another range) when there's only one notional range, unless I'm missing something (such as there being useful reasons to access the StringSet's nodes directly, perhaps?)<br><br>Anyway, probably all an artifact of using inheritance to create StringSet from StringMap which makes things like this awkward.<br><br>- Dave</div></div></div>