<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 18, 2014 at 10:54 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2014 Nov 18, at 10:14, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> 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.<br>
><br>
> 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).<br>
<br>
</span>I agree that the same tradeoff is made for all testing.  Every new test<br>
adds a burden for all developers until the test is deleted, since it adds<br>
time to every single make check call, and needs to be analyzed and<br>
updated every time it fails spuriously.  On the other hand, if it will<br>
prevent real-world regressions, it saves an enormous amount of time.<br>
<br>
For a test to be worthwhile, it needs to save more time than it costs.<br>
<br>
IMO, "does it compile" tests rarely meet this criterium.  It's rare for<br>
them to fail except at the next API change, and they become yet another<br>
call site to update (or delete).<br>
<span class=""><br>
> 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)<br>
<br>
</span>I agree that the ADT should be tested in isolation, and that<br>
functionality unit tests for corner cases within them are extremely<br>
valuable.<br>
<br>
Still not convinced about this particular case.<br>
<span class=""><br>
> 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).<br>
<br>
</span>I assume it's exposed to allow uses like my motivating commit in r221960.<br></blockquote><div><br>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).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I think LLVM makes this kind of tradeoff a lot -- trading the theoretical<br>
benefits of encapsulation and abstraction for the pragmatic ability to<br>
write correct code quickly.</blockquote><div><br>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. </div></div><br></div></div>