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

[David Blaikie via llvm-dev]

On Thu, Dec 29, 2016 at 10:04 AM Mehdi Amini via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

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

I don't think Piotr is suggesting "always use push_back" but "always use
push_back when it's valid" & I'd second this.

To show a simpler example:

std::unique_ptr<T> u(v);

std::unique_ptr<T> u = v;

I'd generally advocate for using copy init (the second form - it doesn't
copy in this case, it moves) over direct init (the first form) because it's
more legible - copy init can only execute implicit ctor operations, whereas
direct init can invoke implicit and explicit operations. So from a
readability perspective seeing the latter is easier than seing the former -
I know the operation is further constrained to simpler/less interesting
operations (eg: 'v' can't be a raw pointer in the second form, it might be
(& then I have to worry about whether that's an ownership transfer that's
intended), etc)

& push_back V emplace_back is the same choice - push_back means only
implicit ops are used and so it's more readable, emplace_back requires more
careful attention.

I think this is a reasonably good stylistic rule - but I'm happy leaving an
open to judgment for sure - there might be readability reasons that an
emplace_back may work better in some cases.

I'd think of this like "else after return" - we don't have any enforcement,
sometimes we make judgment about it being better (for consistency -
sometimes there's a pattern such that else makes the code more readable),
we haven't gone back to do any cleanup of the codebase, but it's pretty
consistently applied/desired in code review.

All that said - sure, I'd like to have tools to help me get this right
(else after return and "prefer copy init style over direct init") & make
that really easy to do.

- Dave


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

Not sure I follow this - we have lots of things we don't currently do with
a tool that's still part of our style & stuff we try to catch with code
review, etc. (else after return is my best example)


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

Sure - but it generalizes further than that, it's just a nice simple
example.

std::vector<int> v(i);

'i' could be an int & this would make a vector of that many ints, or it
could be another vector & this is making a copy, etc.

std::vector<int> v = i;

I know taht's not creating a vector of 'i' many ints.

Yeah, not a great example either - but the general idea applies, the latter
form is less powerful & so is easier to read because you don't have to
worry/think about it as much.

I think we mostly do this sort of thing out of habit anyway - but codifying
it (& if possible, tools to help) could be nice.



>
>
> —
> Mehdi
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/02bea369/attachment.html>