<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 13, 2014 at 4:51 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"><span><br>
> On 2014-Nov-13, at 16:41, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">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>
</span>I'm not convinced this is necessary when there are in-tree users,<br>
but I suppose it can't hurt anything.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><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>
</span>This hardly adds new functionality. The lvalue version collides with the<br>
`Create(StringRef, AllocatorTy&)` overload. </blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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></blockquote><div><br>Really? Lots of them use the default, so far as I can see.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Maybe this overload should be deleted entirely?<br></blockquote><div><br>Maybe - but it seems it's used more than the other.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><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>
</div></div>In particular, this test would fail:<br>
<br>
Immovable I;<br>
StringMapEntry<MoveOnly>::Create(Key, I)<br></blockquote><div><br></div><div>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</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><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" target="_blank">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>
</div></div></blockquote></div><br></div></div>