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

[David Blaikie via llvm-commits]

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 <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().
>

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.


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