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

[Piotr Padlewski via cfe-dev]

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/cfe-dev/attachments/20161229/ea784cca/attachment.html>