<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><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">dblaikie@gmail.com</a>></span> wrote:<br></div><div class="gmail_quote"><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">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">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><div>I guess, the very <a href="http://clang.llvm.org/extra/clang-tidy/checks/misc-unused-parameters.html">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. 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><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><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><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><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><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">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">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">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">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">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer">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><br></div></div>