[PATCH] D17920: Query the StringMap only once when creating MDString (NFC)

[Mehdi Amini via llvm-commits]

> On Mar 22, 2016, at 9:30 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Mar 22, 2016 at 9:17 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> 
> 
> On Tue, Mar 22, 2016 at 9:02 AM, Mehdi AMINI via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> joker.eph added a comment.
> 
> > std::unordered_map::emplace returns the classic <iterator, bool> pair. Perhaps emplace_second should do the same? Rather than relying on being able to detect the invalid default constructed state of the key?
> 
> 
> I don't understand what you mean: `std::pair<iterator, bool> emplace_second(StringRef Key, ArgsTy &&... Args) `
> 
> Right, to match/parallel with:
> 
> template< class... Args >
> std::pair <http://en.cppreference.com/w/cpp/utility/pair><iterator,bool> emplace( Args&&... args )
> 
> Sorry - my mistake, I see (as you pointed out on IRC) that emplace_second already returns the expected pair.
> 
> I'd just hastily misread the code in MDString::get (seeing the access of the value before if test and then misread/assumed the value was used in the if test, instead of the bool in the pair - not an uncommon idiom (when the value has an obvious default value that's not valid for the use case))
>  
> 
>  
> 
> 
> ================
> Comment at: include/llvm/IR/Metadata.h:595
> @@ -594,3 +594,3 @@
>    MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {}
> -  MDString(MDString &&) : Metadata(MDStringKind, Uniqued) {}
> +  MDString(MDString &&R) = default;
> 
> ----------------
> dblaikie wrote:
> > Why do we need a move ctor at all if we're supporting emplace? Can we just delete it? (even if we omit it, it should be removed implicitly - same with the move assignment above, I think)
> StringMap requires movable types (for std::pair I think).
> 
> If we're supporting emplace, it's usually because the identity of the object is important from the moment of construction - so perhaps we should be making sure that identity is actually preserved, same as the standard emplace.
> 
> Still interested in this ^

So, the move ctor is need by the StringMap for other purpose than emplace_second(). Here I fix the move ctor to be actually a move which it was not. 
I could move this to a separate patch, the only reason it is here is that in the original implementation I had to make it public, and I moved it back to private but left the "implementation" change.

-- 
Mehdi




>  
>  
> 
> 
> 
> http://reviews.llvm.org/D17920 <http://reviews.llvm.org/D17920>
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160322/1ce67a35/attachment.html>