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

Hi everyone,
I would like to start a discussion about enforcing use of clang-tidy (or
better clang-tidy-diff) on patches before sending it to review.
I like how clang-format simplified sending patches and reviews, e.g. "Use
clang-format" instead of giving long list of lines that should be formatted
by reviewer.
I believe that clang-tidy can be also be very helpful here.
Note that by enforcing I mean the same way as we enforce clang-format - It
is not about "every changed line need to be formatted by clang-format"
because thee might be some special formatting that looks better
formatted by hand, it is about saving time for 99.9% of the lines. The same
applies to clang-tidy, where there might be some bugs or false positives,
but I would like to save my time as reviewer to not mention
things like "use auto", "use range loop", "pass by
value" over and over.

But before enforcing clang-tidy, we have to get to consensus on adding new
rules to coding style.
I will list clang-tidy checks that I think should be used in LLVM. To make
it short I will not justify why it is better to use this style because the
documentation of checks is doing good job there.

List of checks that I would like to add to .clang-tidy config
modernize-*

http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html


I skipped some checks because they work only in C++14.
Coding style right now only mention using auto.
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
Loops are mentioned here:
http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
- but thi probably should be changed to "use for range loop if possible.
Try to not evaluate end() multiple times if for range loop is not possible
to use"


I don't know which of the listed changes sounds controversial to you, so if
you would like to start discussion about one of them, then please also
mention that you agree/disagree with the others.
One of the things that I would like to propose, is using push_back whenever
the inserted type is the same or it implicitly casts itself.

std::vector<Value*> v;
Instr *I;
Value *V;
IntrinsicInstr *II;
v.push_back(I);
v.push_back(V);
v.push_back(II);

Use emplace_back only if we insert temporary object like:

std::vector<SomeType> v2;
v2.emplace_back(a, b, c); // instead of v.push_back(SomeType(a, b, c));

The first reason is make code simple and more readable. I belive that code
'v.push_back(I)' is more readable than 'v.emplace_back(I)'
because I don't
have to think about if the element type has special constructor taking
Instr, or it is just vector of Instr.
>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.
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).".

In the case of push_back/emplace_back for the same type, there should be no
performance changes, however emplace_back might generate more template
instantiations slowing down compilation.

There is also topic of using insert/emplace for maps showing that
map.emplace can be slower than map.insert. I would not want to distinguish
between emplace for maps and emplace for vectors,

so I believe using emplace for constructed temporaries, even if there would
be some slightly performance regression, looks simpler and more readable.



I am happy to change to write changes to LLVM coding standards document,
but I firstly would like to see what community thinks about it.

NOTE that I don't propose running clang-tidy and modifying whole codebase.
It might happen some day, but we firstly need to agree that these changes
are better.

There are also other checks from performance and misc that I would like to
enable. Some of them are already enabled (all misc), but because they are
more about finding bugs, I would like to narrow discussion only to
modernize.

Piotr
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/ea784cca/attachment-0001.html>

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

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"



> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/5a78799e/attachment.html>

2016-12-29 14:54 GMT+01:00 Daniel Berlin <dberlin at dberlin.org>:
>
>> 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.
>
> 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"
>
> Sure, that's why we have clang-tidy. I propose using push_back
everywhere
except when temporary is passed, where using emplace_back would simplify by
not having to write type name.
modernize-use-emplace can find places like push_back(Type(a, b, c)) and
transform it into emplace_back(a, b, c);
I hand someone a patches for LLVM and clang that
1) modifies every push_back to emplace_back
2) modifies every emplace_back to push_back (if it is valid)
3) modifies every push_back(Type(a, b)) to emplace_back(a, b);

I actually wanted to do this last time at google because you have some
framework to test performance of clang, but I didn't have enough time.
I only have compared time of running tests vs running tests after 3), and
the results were almost the same.
That's why I think that performance is not concern here.
So if someone with such framework would like to help me, then we could
check if there are any performance wins.


>
>
>
>> 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.
>
> But there is still problem with initializer lists, bit-fields, static
members and nulls. When I was testing this check it actually found many of
these cases in LLVM (specially bit fields and initializer lists).
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/af587dba/attachment.html>

I'm a bit confused by this whole discussion.

clang-format is neither mandated (by documentation) nor enforced (by any
infrastructure/automation) for use in the LLVM project that I know of.

It's convenient, and in review people may reasonably ask authors to
consider running it, etc - but we have no system that requires or checks
for that. Might be nice, might not be.

It sounds like even if clang-format were mandated - we mostly accept that
such a mandate is forward-looking, and that we don't want/intend to
reformat the entire existing codebase. So I imagine the same would apply to
clang-tidy (we wouldn't expect to tidy the entire codebase - we'd just
use
clang-tidy as advisory for any new changes as we do with clang-format) - so
I don't think it'd run up against Danny's protest about choosing a
philosophy for wide scale changes, because there would be no wide scale
changes.

All that said, I think like clang-format we should probably have a
project-wide config that specifies which checks we care about - but the
real impediment to adoption of those checks is a streamlined process for
running them on a patch. clang-format has nice integration with my editor
(vim) and source control system (git-clang-format). I have yet to discover
an equally convenient place to put clang-tidy in my workflow. Without that
I doubt it'll see very much adoption within the LLVM community.

- Dave

On Thu, Dec 29, 2016 at 4:24 AM Piotr Padlewski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Hi everyone,
> I would like to start a discussion about enforcing use of clang-tidy (or
> better clang-tidy-diff) on patches before sending it to review.
> I like how clang-format simplified sending patches and reviews, e.g.
"Use
> clang-format" instead of giving long list of lines that should be
formatted
> by reviewer.
> I believe that clang-tidy can be also be very helpful here.
> Note that by enforcing I mean the same way as we enforce clang-format - It
> is not about "every changed line need to be formatted by
clang-format"
> because thee might be some special formatting that looks better
> formatted by hand, it is about saving time for 99.9% of the lines. The
> same applies to clang-tidy, where there might be some bugs or false
> positives, but I would like to save my time as reviewer to not mention
> things like "use auto", "use range loop", "pass by
value" over and over.
>
> But before enforcing clang-tidy, we have to get to consensus on adding new
> rules to coding style.
> I will list clang-tidy checks that I think should be used in LLVM. To make
> it short I will not justify why it is better to use this style because the
> documentation of checks is doing good job there.
>
> List of checks that I would like to add to .clang-tidy config
> modernize-*
>
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html
>
>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html
>
>
> I skipped some checks because they work only in C++14.
> Coding style right now only mention using auto.
>
>
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> Loops are mentioned here:
>
>
http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
> - but thi probably should be changed to "use for range loop if
possible.
> Try to not evaluate end() multiple times if for range loop is not possible
> to use"
>
>
> I don't know which of the listed changes sounds controversial to you,
so
> if you would like to start discussion about one of them, then please also
> mention that you agree/disagree with the others.
> One of the things that I would like to propose, is using push_back
> whenever the inserted type is the same or it implicitly casts itself.
>
> std::vector<Value*> v;
> Instr *I;
> Value *V;
> IntrinsicInstr *II;
> v.push_back(I);
> v.push_back(V);
> v.push_back(II);
>
> Use emplace_back only if we insert temporary object like:
>
> std::vector<SomeType> v2;
> v2.emplace_back(a, b, c); // instead of v.push_back(SomeType(a, b, c));
>
> The first reason is make code simple and more readable. I belive that code
> 'v.push_back(I)' is more readable than 'v.emplace_back(I)'
because I don't
> have to think about if the element type has special constructor taking
> Instr, or it is just vector of Instr.
> 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.
> 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).".
>
> In the case of push_back/emplace_back for the same type, there should be
> no performance changes, however emplace_back might generate more template
> instantiations slowing down compilation.
>
> There is also topic of using insert/emplace for maps showing that
> map.emplace can be slower than map.insert. I would not want to distinguish
> between emplace for maps and emplace for vectors,
>
> so I believe using emplace for constructed temporaries, even if there
> would be some slightly performance regression, looks simpler and more
> readable.
>
>
>
> I am happy to change to write changes to LLVM coding standards document,
> but I firstly would like to see what community thinks about it.
>
> NOTE that I don't propose running clang-tidy and modifying whole
codebase.
> It might happen some day, but we firstly need to agree that these changes
> are better.
>
> There are also other checks from performance and misc that I would like to
> enable. Some of them are already enabled (all misc), but because they are
> more about finding bugs, I would like to narrow discussion only to
> modernize.
>
> 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/20161229/16f1b20c/attachment.html>

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

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.

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


— 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/f50100b9/attachment.html>

On Thu, Dec 29, 2016 at 9:34 AM, David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I'm a bit confused by this whole discussion.
>
> clang-format is neither mandated (by documentation) nor enforced (by any
> infrastructure/automation) for use in the LLVM project that I know of.
>
> It's convenient, and in review people may reasonably ask authors to
> consider running it, etc - but we have no system that requires or checks
> for that. Might be nice, might not be.
>
> It sounds like even if clang-format were mandated - we mostly accept that
> such a mandate is forward-looking, and that we don't want/intend to
> reformat the entire existing codebase. So I imagine the same would apply to
> clang-tidy (we wouldn't expect to tidy the entire codebase - we'd
just use
> clang-tidy as advisory for any new changes as we do with clang-format) - so
> I don't think it'd run up against Danny's protest about
choosing a
> philosophy for wide scale changes, because there would be no wide scale
> changes.
>
> All that said, I think like clang-format we should probably have a
> project-wide config that specifies which checks we care about - but the
> real impediment to adoption of those checks is a streamlined process for
> running them on a patch. clang-format has nice integration with my editor
> (vim) and source control system (git-clang-format). I have yet to discover
> an equally convenient place to put clang-tidy in my workflow. Without that
> I doubt it'll see very much adoption within the LLVM community.
>
+1

I don't remember there being a discussion analogous to the
"enforcement"
aspect of thread for clang-format. The uptake of clang-format was organic.

Piotr, what is different about the situation with clang-tidy (vs
clang-format) that we need to have any discussion of "enforcement" in
this
thread? I think it's totally reasonable to have a discussion of which
checks we want to run by default for the "LLVM style" (to use
clang-format's terminology), and this thread should probably focus on that.

wrt adding rules to the coding standards, I don't think we want to clutter
the coding standards document with a specific rule for every clang-tidy
check we want to have on by default for the "LLVM style", the same way
that
we don't have a rule in the coding standards document for every
clang-format config value which happens to be set in the "LLVM style"
(i.e.
all the stuff you see when you do -dump-config). Just the main ones is fine.

-- Sean Silva

>
> - Dave
>
> On Thu, Dec 29, 2016 at 4:24 AM Piotr Padlewski via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Hi everyone,
>> I would like to start a discussion about enforcing use of clang-tidy
(or
>> better clang-tidy-diff) on patches before sending it to review.
>> I like how clang-format simplified sending patches and reviews, e.g.
"Use
>> clang-format" instead of giving long list of lines that should be
formatted
>> by reviewer.
>> I believe that clang-tidy can be also be very helpful here.
>> Note that by enforcing I mean the same way as we enforce clang-format -
>> It is not about "every changed line need to be formatted by
clang-format"
>> because thee might be some special formatting that looks better
>> formatted by hand, it is about saving time for 99.9% of the lines. The
>> same applies to clang-tidy, where there might be some bugs or false
>> positives, but I would like to save my time as reviewer to not mention
>> things like "use auto", "use range loop",
"pass by value" over and over.
>>
>> But before enforcing clang-tidy, we have to get to consensus on adding
>> new rules to coding style.
>> I will list clang-tidy checks that I think should be used in LLVM. To
>> make it short I will not justify why it is better to use this style
because
>> the documentation of checks is doing good job there.
>>
>> List of checks that I would like to add to .clang-tidy config
>> modernize-*
>>
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> deprecated-headers.html
>>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
>>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> pass-by-value.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> redundant-void-arg.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> replace-auto-ptr.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> shrink-to-fit.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> use-bool-literals.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> use-default-member-init.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> use-equals-default.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-
>> use-equals-delete.html
>>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
>>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
>>
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
>> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html
>>
>>
>> I skipped some checks because they work only in C++14.
>> Coding style right now only mention using auto.
>> http://llvm.org/docs/CodingStandards.html#use-auto-
>> type-deduction-to-make-code-more-readable
>> Loops are mentioned here:
>>
http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-
>> through-a-loop - but thi probably should be changed to "use for
range
>> loop if possible. Try to not evaluate end() multiple times if for range
>> loop is not possible to use"
>>
>>
>> I don't know which of the listed changes sounds controversial to
you, so
>> if you would like to start discussion about one of them, then please
also
>> mention that you agree/disagree with the others.
>> One of the things that I would like to propose, is using push_back
>> whenever the inserted type is the same or it implicitly casts itself.
>>
>> std::vector<Value*> v;
>> Instr *I;
>> Value *V;
>> IntrinsicInstr *II;
>> v.push_back(I);
>> v.push_back(V);
>> v.push_back(II);
>>
>> Use emplace_back only if we insert temporary object like:
>>
>> std::vector<SomeType> v2;
>> v2.emplace_back(a, b, c); // instead of v.push_back(SomeType(a, b, c));
>>
>> The first reason is make code simple and more readable. I belive that
>> code 'v.push_back(I)' is more readable than
'v.emplace_back(I)' because I
>> don't have to think about if the element type has special
constructor
>> taking Instr, or it is just vector of Instr.
>> 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.
>> 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).".
>>
>> In the case of push_back/emplace_back for the same type, there should
be
>> no performance changes, however emplace_back might generate more
template
>> instantiations slowing down compilation.
>>
>> There is also topic of using insert/emplace for maps showing that
>> map.emplace can be slower than map.insert. I would not want to
distinguish
>> between emplace for maps and emplace for vectors,
>>
>> so I believe using emplace for constructed temporaries, even if there
>> would be some slightly performance regression, looks simpler and more
>> readable.
>>
>>
>>
>> I am happy to change to write changes to LLVM coding standards
document,
>> but I firstly would like to see what community thinks about it.
>>
>> NOTE that I don't propose running clang-tidy and modifying whole
>> codebase. It might happen some day, but we firstly need to agree that
these
>> changes are better.
>>
>> There are also other checks from performance and misc that I would like
>> to enable. Some of them are already enabled (all misc), but because
they
>> are more about finding bugs, I would like to narrow discussion only to
>> modernize.
>>
>> Piotr
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161229/70b9063d/attachment-0001.html>