[cfe-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy

[Piotr Padlewski via cfe-dev]

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

>
>> From yesterday discussion, Daniel Berlin proposed using emplace_back
>> everywhere to make code easier to maintain. I think it is valid argument,
>> but I believe it would reduce readability.
>>
>
> Just to be clear; I proposed not trying to switch uses back and forth
> without real data, and to come to some agreement about what should be
> written in the first place, preferably based on real data, and then,
> switching in some organized fashion, not just randomly :)
>
> IE Either have a clear coding standard and enforce it, or leave uses alone
> one way or the other without some demonstration that it actually matters.
>
> I would *personally* prefer to just use emplace_back everywhere, and not
> try to switch between the two, without demonstration of real benefit.
>
> (i'd also accept the other way, use push_back everywhere, and not try to
> switch between the two, without demonstration of real benefit).
>
> This preference is based on a simple truth: People suck at deciding when
> to use one or the other. Yes, i know how to use it and when to use it.
> Lots of our types are cheaply movable, etc, so you probably won't see any
> performance difference even when using them "right".  People have enough
> style/etc issues without having to try to think hard about this.
>
> So that isn't what I would necessarily propose for LLVM.
>
> For LLVM, my answer would be "either make tool do what we want, force use
> of tool, or be consistent and obvious with what we want and then fix it if
> it leads to performance issue"
>
> Sure, that's why we have clang-tidy. I propose using push_back everywhere
except when temporary is passed, where using emplace_back would simplify by
not having to write type name.
modernize-use-emplace can find places like push_back(Type(a, b, c)) and
transform it into emplace_back(a, b, c);
I hand someone a patches for LLVM and clang that
1) modifies every push_back to emplace_back
2) modifies every emplace_back to push_back (if it is valid)
3) modifies every push_back(Type(a, b)) to emplace_back(a, b);

I actually wanted to do this last time at google because you have some
framework to test performance of clang, but I didn't have enough time.
I only have compared time of running tests vs running tests after 3), and
the results were almost the same.
That's why I think that performance is not concern here.
So if someone with such framework would like to help me, then we could
check if there are any performance wins.



>
>
>
>> There is also other reason - emplace_back can't take arguments of some
>> types like bitfields, NULL, static member. Using emplace can also lead to
>> memory leak in case of smart ptrs
>> "
>>
>> auto *ptr = new int(1);v.push_back(std::unique_ptr<int>(ptr));
>>
>> This is because replacing it with emplace_back could cause a leak of
>> this pointer if emplace_back would throw exception before emplacement
>> (e.g. not enough memory to add new element).".
>>
>
> This seems, IMHO, not a useful argument for LLVM since it does not try to
> use exception based error handling to recover.
>
> But there is still problem with initializer lists, bit-fields, static
members and nulls. When I was testing this check it actually found many of
these cases in LLVM (specially bit fields and initializer lists).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20161229/af587dba/attachment.html>