<div dir="ltr">I'm a bit confused by this whole discussion.<br><br>clang-format is neither mandated (by documentation) nor enforced (by any infrastructure/automation) for use in the LLVM project that I know of.<br><br>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.<br><br>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.<br><br>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.<br><br>- Dave<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 29, 2016 at 4:24 AM Piotr Padlewski via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg">Hi everyone,<div class="gmail_msg">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 class="gmail_msg">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 class="gmail_msg">I believe that clang-tidy can be also be very helpful here.</div><div class="gmail_msg">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 class="gmail_msg">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 class="gmail_msg">things like "use auto", "use range loop", "pass by value" over and over. </div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">But before enforcing clang-tidy, we have to get to consensus on adding new rules to coding style.</div><div class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">List of checks that I would like to add to .clang-tidy config</div><div class="gmail_msg">modernize-*</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html</a><br class="gmail_msg"></div><div class="gmail_msg"><a href="http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html</a><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">I skipped some checks because they work only in C++14.</div><div class="gmail_msg">Coding style right now only mention using auto.</div><div class="gmail_msg"><a href="http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable" class="gmail_msg" target="_blank">http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable</a><br class="gmail_msg"></div><div class="gmail_msg">Loops are mentioned here:</div><div class="gmail_msg"><a href="http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop" class="gmail_msg" target="_blank">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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 class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">std::vector<Value*> v;</div><div class="gmail_msg">Instr *I;</div><div class="gmail_msg">Value *V;</div><div class="gmail_msg">IntrinsicInstr *II;</div><div class="gmail_msg">v.push_back(I);</div><div class="gmail_msg">v.push_back(V);</div><div class="gmail_msg">v.push_back(II);</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Use emplace_back only if we insert temporary object like:<br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">std::vector<SomeType> v2;</div><div class="gmail_msg">v2.emplace_back(a, b, c); // instead of v.push_back(SomeType(a, b, c));</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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 class="gmail_msg">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 class="gmail_msg">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 class="gmail_msg">"</div><div class="gmail_msg"><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)" class="gmail_msg"><span class="m_-5133668563107372065gmail-k gmail_msg" style="color:rgb(0,112,32);font-weight:bold">auto</span> <span class="m_-5133668563107372065gmail-o gmail_msg" style="color:rgb(102,102,102)">*</span><span class="m_-5133668563107372065gmail-n gmail_msg">ptr</span> <span class="m_-5133668563107372065gmail-o gmail_msg" style="color:rgb(102,102,102)">=</span> <span class="m_-5133668563107372065gmail-k gmail_msg" style="color:rgb(0,112,32);font-weight:bold">new</span> <span class="m_-5133668563107372065gmail-kt gmail_msg" style="color:rgb(144,32,0)">int</span><span class="m_-5133668563107372065gmail-p gmail_msg">(</span><span class="m_-5133668563107372065gmail-mi gmail_msg" style="color:rgb(64,160,112)">1</span><span class="m_-5133668563107372065gmail-p gmail_msg">);</span>
<span class="m_-5133668563107372065gmail-n gmail_msg">v</span><span class="m_-5133668563107372065gmail-p gmail_msg">.</span><span class="m_-5133668563107372065gmail-n gmail_msg">push_back</span><span class="m_-5133668563107372065gmail-p gmail_msg">(</span><span class="m_-5133668563107372065gmail-n gmail_msg">std</span><span class="m_-5133668563107372065gmail-o gmail_msg" style="color:rgb(102,102,102)">::</span><span class="m_-5133668563107372065gmail-n gmail_msg">unique_ptr</span><span class="m_-5133668563107372065gmail-o gmail_msg" style="color:rgb(102,102,102)"><</span><span class="m_-5133668563107372065gmail-kt gmail_msg" style="color:rgb(144,32,0)">int</span><span class="m_-5133668563107372065gmail-o gmail_msg" style="color:rgb(102,102,102)">></span><span class="m_-5133668563107372065gmail-p gmail_msg">(</span><span class="m_-5133668563107372065gmail-n gmail_msg">ptr</span><span class="m_-5133668563107372065gmail-p gmail_msg">));</span></pre></div><div class="gmail_msg"><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px" class="gmail_msg">This is because replacing it with <code class="m_-5133668563107372065gmail-docutils m_-5133668563107372065gmail-literal gmail_msg" style="background-color:rgb(226,226,226);font-size:1em"><span class="m_-5133668563107372065gmail-pre gmail_msg">emplace_back</span></code> could cause a leak of this pointer if <code class="m_-5133668563107372065gmail-docutils m_-5133668563107372065gmail-literal gmail_msg" style="background-color:rgb(226,226,226);font-size:1em"><span class="m_-5133668563107372065gmail-pre gmail_msg">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" class="gmail_msg">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" class="gmail_msg">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" class="gmail_msg">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" class="gmail_msg"><br class="gmail_msg"></p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px" class="gmail_msg"><br class="gmail_msg"></p><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px" class="gmail_msg">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" class="gmail_msg">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" class="gmail_msg"><span style="font-family:arial,sans-serif;font-size:small;color:rgb(34,34,34)" class="gmail_msg">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 class="gmail_msg"></p></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Piotr</div><div class="gmail_msg"><br class="gmail_msg"></div></div>
_______________________________________________<br class="gmail_msg">
cfe-dev mailing list<br class="gmail_msg">
<a href="mailto:cfe-dev@lists.llvm.org" class="gmail_msg" target="_blank">cfe-dev@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br class="gmail_msg">
</blockquote></div></div>