<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 17, 2014 at 3:17 PM, 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"><div class="HOEnZb"><div class="h5"><br>
> On 2014-Nov-17, at 11:26, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Thu, Nov 13, 2014 at 4:51 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2014-Nov-13, at 16:41, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> ><br>
> > Author: dblaikie<br>
> > Date: Thu Nov 13 18:41:46 2014<br>
> > New Revision: 221958<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=221958&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=221958&view=rev</a><br>
> > Log:<br>
> > StringMap: Test and finish off supporting perfectly forwarded values in StringMap operations.<br>
> ><br>
> > Followup to r221946.<br>
><br>
> I'm not convinced this is necessary when there are in-tree users,<br>
> but I suppose it can't hurt anything.<br>
><br>
> Easier to iterate when someone goes to refactor it & ensures the codepaths continue to be tested even if a certain use case later gets refactored to not use that feature anymore.<br>
><br>
><br>
> ><br>
> > Modified:<br>
> > llvm/trunk/include/llvm/ADT/StringMap.h<br>
> > llvm/trunk/unittests/ADT/StringMapTest.cpp<br>
> ><br>
> > Modified: llvm/trunk/include/llvm/ADT/StringMap.h<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=221958&r1=221957&r2=221958&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=221958&r1=221957&r2=221958&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/include/llvm/ADT/StringMap.h (original)<br>
> > +++ llvm/trunk/include/llvm/ADT/StringMap.h Thu Nov 13 18:41:46 2014<br>
> > @@ -170,9 +170,9 @@ public:<br>
> ><br>
> > /// Create - Create a StringMapEntry with normal malloc/free.<br>
> > template<typename InitType><br>
> > - static StringMapEntry *Create(StringRef Key, InitType InitVal) {<br>
> > + static StringMapEntry *Create(StringRef Key, InitType &&InitVal) {<br>
> > MallocAllocator A;<br>
> > - return Create(Key, A, std::move(InitVal));<br>
> > + return Create(Key, A, std::forward<InitType>(InitVal));<br>
> > }<br>
><br>
> This hardly adds new functionality. The lvalue version collides with the<br>
> `Create(StringRef, AllocatorTy&)` overload.<br>
><br>
> Which no one is using? (or I would've just made the code fail to compile? depending on the exact nature of the collision) Again, why I love having tests - if it's required functionality, I should've broken something.<br>
><br>
<br>
</div></div>Right, but you've extended functionality from my prior commit, and<br>
added tests for what you're guessing my purpose was. It's a rather<br>
awkward way for us to co-develop ;).<br>
<br>
I still think a test that "this API will compile" isn't useful. It<br>
permanently adds time to everyone's build, and doesn't catch any<br>
regressions (at least, none that aren't caught by other in-tree<br>
users failing to compile).<br></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It would have saved us both time if I hadn't broken the commit up<br>
into two, so perhaps I should just collapse them in the future when<br>
I think a unit test is harmful.<br></blockquote><div><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> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> This will *only* add<br>
> functionality for a temporary being passed in. (See my comment below.)<br>
><br>
> A workaround would be to invert the `StringRef` and `AllocatorTy` args,<br>
> but really you should always be passing the map's allocator.<br>
><br>
> Really? Lots of them use the default, so far as I can see.<br>
><br>
> Maybe this overload should be deleted entirely?<br>
><br>
> Maybe - but it seems it's used more than the other.<br>
><br>
<br>
</span>I must have had a `grep`-fail :(. It looked to me like there were<br>
almost no users of this, most users went through the `StringMap`.<br></blockquote><div><br>Hmm, nope - could've been my mistake - I don't really see any uses of StringMapEntry<>::Create - which seems good to me. I just meant that since I was able to introduce that ambiguity without breaking any builds there mustn't be any uses of that functionality. (& I thought I'd seen a variety of uses of raw StringMapEntry::Create before - but maybe I was just recalling the unit tests)</div><div><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> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> ><br>
> > static StringMapEntry *Create(StringRef Key) {<br>
> > @@ -367,8 +367,9 @@ public:<br>
> > /// exists, return it. Otherwise, default construct a value, insert it, and<br>
> > /// return.<br>
> > template <typename InitTy><br>
> > - MapEntryTy &GetOrCreateValue(StringRef Key, InitTy Val) {<br>
> > - return *insert(std::make_pair(Key, std::move(Val))).first;<br>
> > + MapEntryTy &GetOrCreateValue(StringRef Key, InitTy &&Val) {<br>
> > + return *insert(std::pair<StringRef, ValueTy>(<br>
> > + Key, std::forward<InitTy>(Val))).first;<br>
> > }<br>
> ><br>
> > MapEntryTy &GetOrCreateValue(StringRef Key) {<br>
> ><br>
> > Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=221958&r1=221957&r2=221958&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=221958&r1=221957&r2=221958&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)<br>
> > +++ llvm/trunk/unittests/ADT/StringMapTest.cpp Thu Nov 13 18:41:46 2014<br>
> > @@ -256,9 +256,15 @@ TEST_F(StringMapTest, NonDefaultConstruc<br>
> > ASSERT_EQ(iter->second.i, 123);<br>
> > }<br>
> ><br>
> > +struct Immovable {<br>
> > + Immovable() {}<br>
> > + Immovable(Immovable&&) LLVM_DELETED_FUNCTION; // will disable the other special members<br>
> > +};<br>
> > +<br>
> > struct MoveOnly {<br>
> > int i;<br>
> > MoveOnly(int i) : i(i) {}<br>
> > + MoveOnly(const Immovable&) : i(0) {}<br>
> > MoveOnly(MoveOnly &&RHS) : i(RHS.i) {}<br>
> > MoveOnly &operator=(MoveOnly &&RHS) {<br>
> > i = RHS.i;<br>
> > @@ -270,7 +276,7 @@ private:<br>
> > MoveOnly &operator=(const MoveOnly &) LLVM_DELETED_FUNCTION;<br>
> > };<br>
> ><br>
> > -TEST_F(StringMapTest, MoveOnlyKey) {<br>
> > +TEST_F(StringMapTest, MoveOnly) {<br>
> > StringMap<MoveOnly> t;<br>
> > t.GetOrCreateValue("Test", MoveOnly(42));<br>
> > StringRef Key = "Test";<br>
> > @@ -278,6 +284,14 @@ TEST_F(StringMapTest, MoveOnlyKey) {<br>
> > ->Destroy();<br>
> > }<br>
> ><br>
> > +TEST_F(StringMapTest, CtorArg) {<br>
> > + StringMap<MoveOnly> t;<br>
> > + t.GetOrCreateValue("Test", Immovable());<br>
> > + StringRef Key = "Test";<br>
> > + StringMapEntry<MoveOnly>::Create(Key, Immovable())<br>
> > + ->Destroy();<br>
><br>
> In particular, this test would fail:<br>
><br>
> Immovable I;<br>
> StringMapEntry<MoveOnly>::Create(Key, I)<br>
><br>
> Agreed.<br>
><br>
> Though on further reflection, I'm not sure this whole line of changes is the right direction - will discuss more on your use-case commit<br>
><br>
><br>
> > +}<br>
> > +<br>
> > TEST_F(StringMapTest, MoveConstruct) {<br>
> > StringMap<int> A;<br>
> > A.GetOrCreateValue("x", 42);<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>