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

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>

Dave pointed out that I didn't complete one aspect of my argument on the
push_back vs. emplace_back:

On Thu, Dec 29, 2016 at 2:04 PM Chandler Carruth <chandlerc at gmail.com>
wrote:
> 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.
>
This in turn makes me prefer push_back(T(...)) over empalce_back(...). I
*like* the constructor being called explicitly in the users code if is an
*explicit* constructor. For things that should be implicit, they already
are with push_back. If the API of the type went out of its way to make a
constructor call explicit, I'd like to see the user code actually calling
it.


The consequence of this is very simple guidelines for programmers to: don't
use emplace_back unless you *must* use it. So for containers of
non-copyable and non-movable types, it can be very useful and important.
But in every other case I would rather see push_back (and if an explicit
constructor call is necessary, an explicit constructor call).

My two cents.
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/e2ac5f9e/attachment.html>

Thanks for very accurate responses.
- I totally agree what Dave and Chandler said about explicit and implicit
operations, this is what I meant my first email.
  I believe there are places like
    v.emplace_back(A, B);
  istead of
    v.push_back(make_pair(A, B));b
  That can make code simpler. I think in cases like this we can leave it
for judgement of contributor.
  Having said that I think the case Chandler and Dave mentioned should be
in LLVM style guide.
  @Daniel does this argument seem good enough to prefer push_back instead
of emplace_back?

- About clang-tidy config (Dave):
  We already have clang-tidy config in LLVM and clang - check
".clang-tidy"
file. I feel tho that people don't use clang-tidy as often as clang-format.
  This might be because clang-tidy didn't have enough features that they
would like. I believe that clang-tidy is doing much better and could really
save a lot of time for reviewers and contributors.
  But to make it happen we either have to change LLVM style guide, so we
won't need to discuss it over and over in reviews, or gain clang-format
trust - it is so good people don't question it.

- About enforcing (Sean)
  Sorry, I used wrong word here. I meant to use it the same way as
clang-format, to trust people they send cleaned code by clang-tidy the same
way we trust them that they used clang-format.
  Reviewer can then ask contributor to run clang-tidy instead of listing
things that contributor should change. We are a little far from having
mandatory running of clang-format and clang-tidy on every patch
automatically.

- About workflow (Dave)
  If I remember correctly there is some integration for clang-tidy for vim
(YCM?). There is also clang-tidy-diff, but maybe we should have
git-clang-tidy to make it even simpler.


2016-12-29 23:19 GMT+01:00 Chandler Carruth via cfe-dev <
cfe-dev at lists.llvm.org>:
> Dave pointed out that I didn't complete one aspect of my argument on
the
> push_back vs. emplace_back:
>
> On Thu, Dec 29, 2016 at 2:04 PM Chandler Carruth <chandlerc at
gmail.com>
> wrote:
>
>> 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.
>>
>
> This in turn makes me prefer push_back(T(...)) over empalce_back(...). I
> *like* the constructor being called explicitly in the users code if is an
> *explicit* constructor. For things that should be implicit, they already
> are with push_back. If the API of the type went out of its way to make a
> constructor call explicit, I'd like to see the user code actually
calling
> it.
>
>
> The consequence of this is very simple guidelines for programmers to:
> don't use emplace_back unless you *must* use it. So for containers of
> non-copyable and non-movable types, it can be very useful and important.
> But in every other case I would rather see push_back (and if an explicit
> constructor call is necessary, an explicit constructor call).
>
> My two cents.
> -Chandler
>
> _______________________________________________
> 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/20161230/c4d66650/attachment.html>