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

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>

On Fri, Dec 30, 2016 at 2:08 AM Piotr Padlewski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> 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.
>
Do you have examples? The only ones i can come up with are the ones where
the push_back variant literally can't compile because the type isn't
movable.

Perhaps it would be useful to break down categories of can happen here...

Case 1: there is one object already -- this is a *conversion* of a type.
- If the author of the conversion made it *implicit*, then
'v.push_back(x)'
just works.
- If the author of the conversion made it *explicit* I would like to see
the name of the type explicitly: 'v.push_back(T(x))'.

Case 2a: There is a collection of objects that are being composed into an
aggregate. We don't have any interesting logic in the constructor, it takes
an initializer list.
- This should work with 'v.push_back({a, b, c})'
- If it doesn't today, we can fix the type's constructors so that it
does.
- Using 'emplace_back' doesn't help much -- you still need {}s to
form the
std::initializer_list in many cases. Pair and tuple are somewhat unusual in
not requiring them.

Case 2b: A specific constructor needs to be called with an argument list.
These arguments are not merely being aggregated but are inputs to a
constructor that contains logic.
- This is analogous to a case called out w.r.t. '{...}' syntax in the
coding standards[1]
- Similar to that rule, I would like to see a *call to the constructor*
rather than hiding it behind 'emplace_back' as this is a function with
interesting logic.
- That means i would write T(a, b, c) anyways, and 'v.push_back(T(a, b,
c))' works.

[1]:
http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor

Case 3: Passing objects of type 'T' through 'push_back' fails to
compile
because they cannot be copied or moved.
- You *must* use 'emplace_back' here. No argument (obviously).

My experience with LLVM code and other codebases is that case 3 should be
extremely rare. The intersection of "types that cannot be moved or
copied"
and "types that you put into containers" is typically small.


Anyways, I don't disagree with this point with a tiny fix:
> I think in cases like this we can leave it for judgement of contributor.
>
*or reviewer*. ;]

I continue to think exceptions can be made in rare cases when folks have
good reasons. But I expect this to be quite rare. =]
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161230/84370edf/attachment.html>

2016-12-30 11:34 GMT+01:00 Chandler Carruth <chandlerc at gmail.com>:
> On Fri, Dec 30, 2016 at 2:08 AM Piotr Padlewski via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> 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.
>>
>
> Do you have examples? The only ones i can come up with are the ones where
> the push_back variant literally can't compile because the type
isn't
> movable.
>
> Perhaps it would be useful to break down categories of can happen here...
>
> Case 1: there is one object already -- this is a *conversion* of a type.
> - If the author of the conversion made it *implicit*, then
> 'v.push_back(x)' just works.
> - If the author of the conversion made it *explicit* I would like to see
> the name of the type explicitly: 'v.push_back(T(x))'.
>
> Case 2a: There is a collection of objects that are being composed into an
> aggregate. We don't have any interesting logic in the constructor, it
takes
> an initializer list.
> - This should work with 'v.push_back({a, b, c})'
> - If it doesn't today, we can fix the type's constructors so that
it does.
> - Using 'emplace_back' doesn't help much -- you still need {}s
to form the
> std::initializer_list in many cases. Pair and tuple are somewhat unusual in
> not requiring them.
>
>  This sounds extremely reasonable.
> Case 2b: A specific constructor needs to be called with an argument list.
> These arguments are not merely being aggregated but are inputs to a
> constructor that contains logic.
> - This is analogous to a case called out w.r.t. '{...}' syntax in
the
> coding standards[1]
> - Similar to that rule, I would like to see a *call to the constructor*
> rather than hiding it behind 'emplace_back' as this is a function
with
> interesting logic.
> - That means i would write T(a, b, c) anyways, and 'v.push_back(T(a, b,
> c))' works.
>
> Calling emplace_back with 0 or multiple arguments is a clear way of saying
"this constructor takes multiple arguments".
We can do it with initializer list with easy way like:
v.emplace_back()        == v.push_back({})
v.emplace_back(a, b ,c) == v.push_back({a, b, c})

I personally never liked the initializer syntax because of tricky casees
like:

vector<string> v{{"abc", "def"}};
Which is equivalent of
vector<string> v = {std::string("abc", "def")};
That will call std::string ctor with 2 iterators likely crashing, and
putting same string might gives us empty string.

In this case programmer probably meant
std::vector<std:string> v({"abc", "def"});
or
std::vector<std::string> v = {"abc", "def"};

But this case is not possible to mess up with push_back (in the case of
vector<vector<string>> or something). At least I hope it is not.
So avoiding braces is my personal preference. It is fine for me if we would
choose to prefer 'v.push_back({a, b, c})' instead of
'v.emplace_back(a, b,
c)', ofc as long as most of the community would prefer first form to the
second :)


[1]: http://llvm.org/docs/CodingStandards.html#do-not-
> use-braced-initializer-lists-to-call-a-constructor
>
> Case 3: Passing objects of type 'T' through 'push_back'
fails to compile
> because they cannot be copied or moved.
> - You *must* use 'emplace_back' here. No argument (obviously).
>
> My experience with LLVM code and other codebases is that case 3 should be
> extremely rare. The intersection of "types that cannot be moved or
copied"
> and "types that you put into containers" is typically small.
>
>
> Anyways, I don't disagree with this point with a tiny fix:
>
>> I think in cases like this we can leave it for judgement of
contributor.
>>
> *or reviewer*. ;]
>
> I continue to think exceptions can be made in rare cases when folks have
> good reasons. But I expect this to be quite rare. =]
>
Piotr
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161230/5fa9fd77/attachment.html>