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

[David Blaikie via llvm-commits]

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 <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 ^


>
>
>>
>>
>>
>> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160322/25eb6f6c/attachment.html>