<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 22, 2016 at 12:14 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">cc-ing the original requester and author of the check for their opinion on the check usage in LLVM.<br></div><span class=""><div class="gmail_quote"><br></div><div class="gmail_quote">On Thu, Apr 14, 2016 at 7:17 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br></div></span><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Apr 13, 2016 at 6:10 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><p dir="ltr">There was no discussion, but I don't anticipate any objections, because this check neither has any support in <a href="http://llvm.org/docs/CodingStandards.html" target="_blank">http://llvm.org/docs/CodingStandards.html</a> nor corresponds to a wide-spread practice in the LLVM code.</p></div></blockquote></span><div>It's the sort of thing I wouldn't really expect to be formalized and that I'd expect the codebase to rot /very/ quickly in the absence of any warning/check, even if it was formalized. So neither of these things seem surprising to me, but I wouldn't necessarily conclude that the check isn't applicable to LLVM because of them. <br></div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><p dir="ltr"> There are a handful of methods using this convention and hundreds that don't.</p></div></blockquote></span><div>"convention" makes it sound deliberate. I imagine most of the dead parameters are just a function of refactoring (or API constraints - I imagine that the check can handle the case of an unused parameter in a virtual override, but may not catch unused parameters where a matching signature is needed for other reasons (calling through a function pointer, etc)? I imagine we might have a handful of those in the codebase, but would be surprised if there are a lot) <br></div></div></div></div></blockquote><div><br></div></span><div>I guess, the very <a href="http://clang.llvm.org/extra/clang-tidy/checks/misc-unused-parameters.html" target="_blank">scarce documentation</a> for this check leads to a certain confusion. The check complains on any named unused parameters, which are present in a large amounts in LLVM code. The check can be muted by commenting out unused parameters: `void f(int x) {} -> void f(int /*x*/)`. By the "convention" I mean systematically commenting out unused parameters. </div></div></div></div></blockquote><div><br></div><div>I would agree that the presence of such comments would indicate a positive convention. But the absence of them doesn't seem to me to indicate a convention in the other direction. It seems to me the absence just indicates a lack of any convention/attempt to clean things up. Without any diagnostics (warnings, tidy checks, etc) unused variables will easily be introduced and never cleaned up. (much like any warning - if it's not checked, even if people try to apply it, they'll miss it a lot & the codebase will tend to have many instances of the problem grow over time)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This seems to be rarely done in LLVM (but this check can mass-fix the code and make it possible to enable -Wunused-parameter by default to avoid regressions).</div><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><p dir="ltr"> This is the single most noisy check on LLVM code that was turned on before this commit. If we want to use this check, we first need to adopt the corresponding rule and clean up thousands of warnings from this check.</p></div></blockquote></span><div>Isn't that (one of) the point(s) of clang-tidy, though - to allow us to hold a higher bar for new code, without having to cleanup the codebase to make it check-clean?<br></div></div></div></div></blockquote><div><br></div></span><div>Yes, it is, but our only way of ignoring old warnings so far is to show warnings on changed line of a patch. While it can work reasonably when there are few warnings in the codebase, it becomes less usable when the number of warnings is large and one has to fix pre-existing warnings rather frequently.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>(side note: I actually haven't seen/heard/understood how clang-tidy is being integrated into any LLVM developer workflow - is it?</div></div></div></div></blockquote><div><br></div></span><div>AFAIK, nobody uses clang-tidy routinely for LLVM code (a conclusion based on the presence of a very noisy check - the one this revision disables - in the default configuration).</div></div></div></div></blockquote><div><br></div><div>So what's the plan here - is there some way that we hope to provide to LLVM developers to use clang-tidy? Could we sort that out before we go cleaning up the codebase? (because any such cleanup, without ongoing encouragement/enforcement seems bound to bitrot & perhaps shouldn't bother cleaning up in the first place?)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> It'd be great to have a few different levels of integration - in the build for the things that just aren't clang diagnostics because they're project specific (not because they have a low false positive or because we don't want to cleanup LLVM first) - actually it'd be great if clang itself had some way to integrate /thoes/ ones. Then another level for the "going forward" kind - just restrict itself to things in my local change so I don't introduce more mistakes of the same kind, etc)<br><br>- Dave</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div>
<div class="gmail_quote">On Apr 13, 2016 19:32, "David Blaikie" <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Was there a thread/discussion/context for this choice?</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 13, 2016 at 1:58 AM, Alexander Kornienko via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: alexfh<br>
Date: Wed Apr 13 03:58:52 2016<br>
New Revision: 266183<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=266183&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=266183&view=rev</a><br>
Log:<br>
Don't use misc-unused-parameters check on LLVM.<br>
<br>
Modified:<br>
llvm/trunk/.clang-tidy<br>
<br>
Modified: llvm/trunk/.clang-tidy<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/.clang-tidy?rev=266183&r1=266182&r2=266183&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/.clang-tidy?rev=266183&r1=266182&r2=266183&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/.clang-tidy (original)<br>
+++ llvm/trunk/.clang-tidy Wed Apr 13 03:58:52 2016<br>
@@ -1,4 +1,4 @@<br>
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,readability-identifier-naming'<br>
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,readability-identifier-naming'<br>
CheckOptions:<br>
- key: readability-identifier-naming.ClassCase<br>
value: CamelCase<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>
</blockquote></div>
</div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>