<div dir="ltr">Hi everyone,<div>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. </div><div>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.</div><div>I believe that clang-tidy can be also be very helpful here.</div><div>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 </div><div>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 </div><div>things like "use auto", "use range loop", "pass by value" over and over. </div><div><br></div><div>But before enforcing clang-tidy, we have to get to consensus on adding new rules to coding style.</div><div>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.</div><div><br></div><div>List of checks that I would like to add to .clang-tidy config</div><div>modernize-*</div><div><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html</a><br></div><div><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html</a><br></div><div><br></div><div><br></div><div>I skipped some checks because they work only in C++14.</div><div>Coding style right now only mention using auto.</div><div><a href="http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable">http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable</a><br></div><div>Loops are mentioned here:</div><div><a href="http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop">http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop</a> - 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"</div><div><br></div><div><br></div><div>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.</div><div>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. </div><div><br></div><div>std::vector<Value*> v;</div><div>Instr *I;</div><div>Value *V;</div><div>IntrinsicInstr *II;</div><div>v.push_back(I);</div><div>v.push_back(V);</div><div>v.push_back(II);</div><div><br></div><div>Use emplace_back only if we insert temporary object like:<br></div><div><br></div><div>std::vector<SomeType> v2;</div><div>v2.emplace_back(a, b, c); // instead of v.push_back(SomeType(a, b, c));</div><div><br></div><div>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.</div><div>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.</div><div>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</div><div>"</div><div><pre style="overflow-x:auto;overflow-y:hidden;border-color:rgb(12,55,98);border-style:dotted;border-width:thin;margin-top:0px;margin-bottom:12px;padding:0.8em;background-color:rgb(240,240,240);color:rgb(51,51,51)"><span class="gmail-k" style="color:rgb(0,112,32);font-weight:bold">auto</span> <span class="gmail-o" style="color:rgb(102,102,102)">*</span><span class="gmail-n">ptr</span> <span class="gmail-o" style="color:rgb(102,102,102)">=</span> <span class="gmail-k" style="color:rgb(0,112,32);font-weight:bold">new</span> <span class="gmail-kt" style="color:rgb(144,32,0)">int</span><span class="gmail-p">(</span><span class="gmail-mi" style="color:rgb(64,160,112)">1</span><span class="gmail-p">);</span>
<span class="gmail-n">v</span><span class="gmail-p">.</span><span class="gmail-n">push_back</span><span class="gmail-p">(</span><span class="gmail-n">std</span><span class="gmail-o" style="color:rgb(102,102,102)">::</span><span class="gmail-n">unique_ptr</span><span class="gmail-o" style="color:rgb(102,102,102)"><</span><span class="gmail-kt" style="color:rgb(144,32,0)">int</span><span class="gmail-o" style="color:rgb(102,102,102)">></span><span class="gmail-p">(</span><span class="gmail-n">ptr</span><span class="gmail-p">));</span></pre></div><div><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">This is because replacing it with <code class="gmail-docutils gmail-literal" style="background-color:rgb(226,226,226);font-size:1em"><span class="gmail-pre">emplace_back</span></code> could cause a leak of this pointer if <code class="gmail-docutils gmail-literal" style="background-color:rgb(226,226,226);font-size:1em"><span class="gmail-pre">emplace_back</span></code> would throw exception before emplacement (e.g. not enough memory to add new element).".</p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">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.</p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">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,</p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">so I believe using emplace for constructed temporaries, even if there would be some slightly performance regression, looks simpler and more readable.</p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px"><br></p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px"><br></p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">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.</p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">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.</p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px"><span style="font-family:arial,sans-serif;font-size:small;color:rgb(34,34,34)">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.</span><br></p></div><div><br></div><div>Piotr</div><div><br></div></div>