[PATCH] D31171: Improve StringMap iterator support.

[David Blaikie via llvm-commits]

On Tue, Mar 21, 2017 at 10:29 AM Zachary Turner via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>

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.


>
> > 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?
>

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?)

Anyway, probably all an artifact of using inheritance to create StringSet
from StringMap which makes things like this awkward.

- Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170321/513faef8/attachment.html>