<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 10, 2014 at 10:22 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><br>
> On 2014 Dec 9, at 11:02, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Dec 9, 2014 at 10:52 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> Author: dexonsmith<br>
> Date: Tue Dec 9 12:52:38 2014<br>
> New Revision: 223806<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=223806&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=223806&view=rev</a><br>
> Log:<br>
> Fix a GCC build failure from r223802<br>
><br>
> Modified:<br>
> llvm/trunk/lib/IR/Metadata.cpp<br>
><br>
> Modified: llvm/trunk/lib/IR/Metadata.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=223806&r1=223805&r2=223806&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=223806&r1=223805&r2=223806&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/IR/Metadata.cpp (original)<br>
> +++ llvm/trunk/lib/IR/Metadata.cpp Tue Dec 9 12:52:38 2014<br>
> @@ -341,7 +341,8 @@ MDString *MDString::get(LLVMContext &Con<br>
> if (I != Store.end())<br>
> return &I->second;<br>
><br>
> - auto *Entry = StringMapEntry<MDString>::Create(Str, Store.getAllocator());<br>
> + auto *Entry =<br>
> + StringMapEntry<MDString>::Create(Str, Store.getAllocator(), MDString());<br>
><br>
> Looks like this whole function could maybe be a bit simpler (& avoid the double lookup on first access, as well as the raw StringMapEntry<>::Create):<br>
><br>
> auto &IterBool = Store.insert(std::make_pair(Str, MDString()));<br>
> if (IterBool.second)<br>
> IterBool.first->second.Entry = &*IterBool.first;<br>
> return &IterBool.first->second;<br>
><br>
> (I'm not even sure it's worth the conditional? Maybe the unconditional assignment is cheaper?)<br>
><br>
> (at some point I think we might be able to avoid exposing the ability to manually create/insert/etc StringMapEntries & pretend, as much as possible, that they're just a pair like a normal map)<br>
<br>
</div></div>I agree that flow is better.<br>
<br>
However, I just implemented it, and it looks to me like it requires a<br>
copy constructor, as well the unfortunate:<br>
<br>
friend struct std::pair<StringRef, MDString>;<br></blockquote><div><br>I'm confused then - doesn't the above code:<br><br><span style="color:rgb(80,0,80)"> + auto *Entry =</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)"> + StringMapEntry<MDString>::</span><span style="color:rgb(80,0,80)">Create(Str, Store.getAllocator(), MDString());</span><br><br></div><div>require a copy (or move) ctor in MDString too?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I don't think this is acceptable. It's really not valid to copy or move<br>
an `MDString` outside of `MDString::get()`, so it's important that<br>
creating a pair out of one is a compile-time error.<br>
<br>
This could be fixed by adding new API to `StringMap` that avoids the use<br>
of `std::pair<>`, but I'm not sure it fits with your vision there.<br>
<br>
Nevertheless, here's the API that I was thinking. In `StringMap`:<br>
<br>
template <class ArgsTy...><br>
std::pair<iterator, bool> emplace_second(StringRef, ArgsTy &&...);<br>
<br>
This emplaces the `mapped_type` with the provided args (open to<br>
suggestions on naming). IMO, this matches `StringMap`'s<br>
specialization around the `key_type` being `StringRef`.<br>
<br>
Thoughts?<br></blockquote><div><br>Ah, coming back around to the discussion from a few weeks back. I'd prefer the actual STL-compatible API of 'emplace()' rather than special-casing the second parameter, of course, though I realize it's a tad more work to wrangle the tuples, etc. (wouldn't mind special casing "piecewise_construct, StringRef, tuple" which would be similar in functionality to your API (but would require tuple wrangling) without having to deal with the full generality of being able to pass the StringRef ctor argument tuple, etc, for limited benefit.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
><br>
> (and I wouldn't mind if this function returned a reference instead of a pointer, but I realize that's just my own aesthetic preference and a lot of work to make the change that might not be better)<br>
<br>
</span>Personally, I agree with your aesthetic here, but the style with IR is to<br>
return a pointer, so it's better to be consistent.</blockquote></div><br></div></div>