[llvm-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy
Piotr Padlewski via llvm-dev
llvm-dev at lists.llvm.org
Thu Dec 29 04:23:50 PST 2016
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
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
I skipped some checks because they work only in C++14.
Coding style right now only mention using auto.
Loops are mentioned here:
- 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
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.
Use emplace_back only if we insert temporary object like:
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
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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-dev