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

Thu Dec 29 14:04:09 PST 2016

Somewhat unfortunately, we have two discussions here:

- Clang-tidy has checks that might improve code -- should we deploy them?
If so which?

I'll address this in a separate email, and focus this one on:

- Should we have coding standards around 'push_back' and 'emplace_back',
and if so, what should they say?

I think the amount of confusion makes a coding standard useful.

As for what it should say, I tend to agree with Dave here. In particular:

On Thu, Dec 29, 2016 at 12:03 PM David Blaikie via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On Thu, Dec 29, 2016 at 11:46 AM Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
> On Dec 29, 2016, at 11:20 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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.
>
>
> Define “valid”? Just that it will compile?
>
>
> Yep
>
>
>
> 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
>
>
> I don’t see what’s more readable about “only implicit ctor”.
>
>
> It limits the set of operations that can be performed by the code. So when
> I read it there's less I have to think about/consider. (& specifically, the
> implicit operations tend to be simpler/safer/more obvious - copy/move or
> operations that are similar - not complex/interesting things like "taking
> ownership from a raw pointer" or "creating a vector of N elements")
>
>
> Emplace is very explicit that we intended to construct an object, I don’t
> understand what hurts readability here?
>
>
> Going back to the example above, given the following two lines:
>
> std::unique_ptr<T> u(foo());
> std::unique_ptr<T> u = foo();
>
> (& the equivalent: emplace_back(foo()) V push_back(foo()) for a vector of
> unique_ptr)
>
> the copy init/push_back are simpler to read because they aren't as
> powerful - I don't have to wonder if something is taking ownership there
> (or if I'm creating a vector of N ints, etc). I know it's a simple/obvious
> operation, generally (because others shouldn't be implicit).
>

This is exactly where I come down as well.

Another useful way to think about it is "what do I need to understand to
understand the semantics of this operation".

In order to understand push_back, I need only read its documentation. The
type going in will have to have value semantics (potentially with moves).
Otherwise it will be an error. And push_back's documentation says what it
does.

In order to understand a given emplace_back call I have to *both* read its
documentation and the documentation for all of the constructors on the type.

Still another way to see the consequence of this is to look at the nature
of compiler errors when a programmer makes a mistake.

With emplace_back, if you fail to call the constructor correctly, all of
the error messages come with a layer (or many layers) of template
instantiation. With push_back(T(...)), the constructor call is direct and
the error messages are as well.

With emplace_back, if you are converting from one type to another and the
conversion fails, you again get template backtraces. With push_back, all
the conversions happen before the push_back method and so the error is
local to your code.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/c0277b4a/attachment.html>