[llvm] r290685 - [NewGVN] replace emplace_back with push_back

[Piotr Padlewski via llvm-commits]

2016-12-29 0:07 GMT+01:00 Daniel Berlin <dberlin at dberlin.org>:

>
>
> On Wed, Dec 28, 2016 at 2:34 PM, Piotr Padlewski <
> piotr.padlewski at gmail.com> wrote:
>
>>
>> 2016-12-28 22:09 GMT+01:00 Daniel Berlin <dberlin at dberlin.org>:
>>
>>>
>>>
>>> On Wed, Dec 28, 2016 at 12:36 PM, Piotr Padlewski via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: prazek
>>>> Date: Wed Dec 28 14:36:08 2016
>>>> New Revision: 290685
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=290685&view=rev
>>>> Log:
>>>> [NewGVN] replace emplace_back with push_back
>>>>
>>>
>>> I'm somewhat against this change.
>>> It would be nice to have coding guidelines here.
>>>
>>>
>> True, I gonna start thread about it soon.
>>
>
> Can you please just revert this and discuss it, and then we decide what to
> do as a whole?
> I'd rather not see it done piecemeal before we decide what to do :P
>
>
Sure, I was doing that to avoid unnecessary comments in the review.
I will start the discussion about modernizing the code style tomorrow.
push_back reverted.


>
>>>> emplace_back is not faster if it is equivalent to push_back. In this
>>>> cases emplaced value had the
>>>> same type that the one stored in container. It is ugly and it might be
>>>> even slower (see
>>>> Scott Meyers presentation about emplacement).
>>>>
>>>
>>> I'm going to disagree about "ugly", i don't think it's ugly or not ugly.
>>> If it's really slower, i'd like to see numbers.
>>> Otherwise, i think we should just be consistent if we can.
>>>
>>>
>> Some numbers:
>> https://youtu.be/smqT9Io_bKo?t=29m16s
>>
>> It is for map insert/emplace.
>>
>
> This is about whether the insertion happens or not.
> That seems very different than the other cases.
>
>
>> It probably doesn't matter much for emplace_back, it only can add extra
>> instantiations,
>> but I guess we don't want to have diffrent rules for using insert/emplace
>> and push_back/emplace_back
>>
>
> We also probably, IMHO, do not want to litter the code with both push_back
> *and* emplace_back if there is no performance loss from emplace_back except
> for maps/sets.
>
> The whole point of code style is consistency, with exceptions made for
> performance, etc.
>
> IMHO push_back are more consistent.

>
>> There is also some useful info here:
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
>>
>> Piotr
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161229/e8406b70/attachment.html>