[llvm] r221958 - StringMap: Test and finish off supporting perfectly forwarded values in StringMap operations.

[David Blaikie]

On Tue, Nov 18, 2014 at 10:54 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2014 Nov 18, at 10:14, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Only if those uses remain in-tree, which isn't always the case.
> Bitrotting behavior because the testing is lost or incomplete isn't great.
> >
> > I agree that "does this compile" tests are of limited value (as much
> here as they are in clang) without testing for particular behavior - which
> could be added. Though adding compile-time because we're testing something
> doesn't seem like an argument not to test - the same tradeoff is made for
> any testing at all (& if we really want to fix that, we'd need some kind of
> dependency based test execution so we don't run tests for unchanged code).
>
> I agree that the same tradeoff is made for all testing.  Every new test
> adds a burden for all developers until the test is deleted, since it adds
> time to every single make check call, and needs to be analyzed and
> updated every time it fails spuriously.  On the other hand, if it will
> prevent real-world regressions, it saves an enormous amount of time.
>
> For a test to be worthwhile, it needs to save more time than it costs.
>
> IMO, "does it compile" tests rarely meet this criterium.  It's rare for
> them to fail except at the next API change, and they become yet another
> call site to update (or delete).
>
> > I'd still argue that the ADT should be tested is isolation - for the
> same reason we have all the existing ADT tests. It ensures they're well
> tested and tested even when we refactor existing code in such a way as to
> not use a certain piece of functionality for a time. (ADT functionality
> tests are obviously really high value - testing corner cases, etc, is much
> easier to debug in a unit test than when an optimization doesn't fire
> correctly because a container's misbehaving)
>
> I agree that the ADT should be tested in isolation, and that
> functionality unit tests for corner cases within them are extremely
> valuable.
>
> Still not convinced about this particular case.
>
> > Which is good - raw uses of StringMapEntry shouldn't really be common
> (it's sort of weird that we expose that in the API at all, to be honest).
>
> I assume it's exposed to allow uses like my motivating commit in r221960.
>

>From the days before we had perfect forwarding, perhaps - but since there
don't appear to be any mentions of StringMap.*Create apart from unit tests
and your use, I do wonder about it's usefulness when there's a more
consistent & compatible API that aligns with other associative containers
(as with many of the other members of StringMap).


> I think LLVM makes this kind of tradeoff a lot -- trading the theoretical
> benefits of encapsulation and abstraction for the pragmatic ability to
> write correct code quickly.


Invoking explicit ctors implicitly like this is likely to be incorrect
pretty easily. The standard containers/API go to some lengths to make sure
that happens in only very specific operations that have names that help the
user know they're invoking a ctor and passing ctor arguments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141118/f425142f/attachment.html>