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

On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Are there any other comments about changing style guide?
> I would like to add points like
>
> - prefer "using' instead of "typedef"
> - use default member initialization
> struct A {
>   void *ptr = nullptr;
> };
>
> (instead of doing it in constructor)
>
> - use default, override, delete
> - skip "virtual" with override
>
> The last point is to get to consensus with
>
> push_back({first, second})
> or
> emplace_back(first ,second);
>

It might be a bit noisy, but I'd be inclined to start a separate thread for
each of these on llvm-dev with a clear subject line relevant to each one.
So they don't get lost and some of them don't get drowned out by the
discussion of others, etc.

>
> 2016-12-30 12:26 GMT+01:00 Piotr Padlewski <piotr.padlewski at
gmail.com>:
>
>
>
> 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
>
>
> _______________________________________________
> 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/20170109/ac0e683d/attachment.html>

2017-01-09 17:24 GMT+01:00 David Blaikie <dblaikie at gmail.com>:
>
>
> On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Are there any other comments about changing style guide?
>> I would like to add points like
>>
>> - prefer "using' instead of "typedef"
>> - use default member initialization
>> struct A {
>>   void *ptr = nullptr;
>> };
>>
>
>> (instead of doing it in constructor)
>>
>> - use default, override, delete
>> - skip "virtual" with override
>>
>> The last point is to get to consensus with
>>
>> push_back({first, second})
>> or
>> emplace_back(first ,second);
>>
>
>
> It might be a bit noisy, but I'd be inclined to start a separate thread
> for each of these on llvm-dev with a clear subject line relevant to each
> one. So they don't get lost and some of them don't get drowned out
by the
> discussion of others, etc.
>
>
Sure, I can do it, but at least now I don't see much of attention of people
to any of the subject, so I would not like to start 5 empty threads.
I guess as long as the thread is not getting noisy I will keep only one.
Does it sound ok?


>
>> 2016-12-30 12:26 GMT+01:00 Piotr Padlewski <piotr.padlewski at
gmail.com>:
>>
>>
>>
>> 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
>>
>>
>> _______________________________________________
>> 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/20170109/b030bc20/attachment.html>

On Mon, Jan 9, 2017 at 10:10 AM Piotr Padlewski <piotr.padlewski at
gmail.com>
wrote:

2017-01-09 17:24 GMT+01:00 David Blaikie <dblaikie at gmail.com>:



On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

Are there any other comments about changing style guide?
I would like to add points like

- prefer "using' instead of "typedef"
- use default member initialization
struct A {
  void *ptr = nullptr;
};


(instead of doing it in constructor)

- use default, override, delete
- skip "virtual" with override

The last point is to get to consensus with

push_back({first, second})
or
emplace_back(first ,second);



It might be a bit noisy, but I'd be inclined to start a separate thread for
each of these on llvm-dev with a clear subject line relevant to each one.
So they don't get lost and some of them don't get drowned out by the
discussion of others, etc.


Sure, I can do it, but at least now I don't see much of attention of people
to any of the subject, so I would not like to start 5 empty threads.
I guess as long as the thread is not getting noisy I will keep only one.
Does it sound ok?


Perhaps - if no one else pipes up *shrug*

If that's the case, I'd at least suggest submitting the changes to the
style guide separately with clear subject/titles so people reading commits
might see/notice/discuss there.

FWIW: I'm also in favor of push_back where valid. Though I'm not sure
it's
so much a matter of votes, but justification, etc. As for the others -
sure, they all sound good to me.

Also - once these are in the style guide there's still a separate
discussion to be had about whether automated cleanup is worthwhile & how
best to go about that sort of thing.






2016-12-30 12:26 GMT+01:00 Piotr Padlewski <piotr.padlewski at gmail.com>:



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


_______________________________________________
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/20170109/828fa08e/attachment.html>