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

[Duncan P. N. Exon Smith]

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

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.