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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Mar-22, at 10:16, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Mar 22, 2016 at 10:03 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
>> 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> wrote:
>> 
>> 
>> On Tue, Mar 22, 2016 at 9:02 AM, Mehdi AMINI via llvm-commits <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<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().
> 
> OK - here's where a unit test would be nice, perhaps, to demonstrate that emplace_second builds exactly one instance of the value and doesn't move it anywhere (and, hopefully, that it can be called without a movable value - but perhaps that's not possible? (growing may involve moving keys?))
>  
> 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.
> 
> *nod* Might be that DenseMap can't support non-movable keys, so it'll be a bit different from emplace on standard maps.

Maybe I'm misreading it (or misremembering), but I don't see why you need
a move constructor now that you construct the MDString in place.

Note that this is a StringMap, not a DenseMap; growing doesn't involve
any moves at all.

>  
> 
> -- 
> Mehdi
> 
> 
> 
> 
>>  
>>  
>> 
>> 
>> 
>> http://reviews.llvm.org/D17920
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits