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
I skipped some checks because they work only in C++14.
Coding style right now only mention using auto.
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.
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 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.