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

[Mehdi Amini via cfe-dev]

> On Dec 29, 2016, at 5:54 AM, Daniel Berlin <dberlin at dberlin.org> wrote:
> 
> 
> 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.

I agree that “people suck at deciding”, me in the first place in the line, and that’s why I like clear and good guideline and stick to it unless "demonstration of real benefit”.
I also think we can have clear guideline that are different from “always use emplace_back” or “always use push_back”, like Piotr is suggesting.

That said, I’m not convinced the relative “ugliness" of emplace_back  (I’m not sure what’s ugly there, I only see it as a habit to take)  vs push_back is enough to justify a set of rules to decide between the two, I’d be fine with “always using emplace_back” in the name of “simple rule is better when possible".

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

+1
Ultimately whatever we do is not practical if it can’t be done by a tool.


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

This, and also we should not write code like that (naked pointers, calling new), using make_unique<> for instance would prevent any such situation. Passing a raw pointer to a container of unique_ptr can be flagged by a tool.


— 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20161229/f50100b9/attachment.html>