[llvm] r266183 - Don't use misc-unused-parameters check on LLVM.

[Alexander Kornienko via llvm-commits]

cc-ing the original requester and author of the check for their opinion on
the check usage in LLVM.

On Thu, Apr 14, 2016 at 7:17 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Wed, Apr 13, 2016 at 6:10 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
>
>> There was no discussion, but I don't anticipate any objections, because
>> this check neither has any support in
>> http://llvm.org/docs/CodingStandards.html nor corresponds to a
>> wide-spread practice in the LLVM code.
>>
> 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.
>
>> There are a handful of methods using this convention and hundreds that
>> don't.
>>
> "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)
>

I guess, the very scarce documentation
<http://clang.llvm.org/extra/clang-tidy/checks/misc-unused-parameters.html> 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).

> 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.
>>
> 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?
>

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.


> (side note: I actually haven't seen/heard/understood how clang-tidy is
> being integrated into any LLVM developer workflow - is it?
>

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).


> 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)
>
> - Dave
>
>
>> On Apr 13, 2016 19:32, "David Blaikie" <dblaikie at gmail.com> wrote:
>>
>>> Was there a thread/discussion/context for this choice?
>>>
>>> On Wed, Apr 13, 2016 at 1:58 AM, Alexander Kornienko via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: alexfh
>>>> Date: Wed Apr 13 03:58:52 2016
>>>> New Revision: 266183
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=266183&view=rev
>>>> Log:
>>>> Don't use misc-unused-parameters check on LLVM.
>>>>
>>>> Modified:
>>>>     llvm/trunk/.clang-tidy
>>>>
>>>> Modified: llvm/trunk/.clang-tidy
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/.clang-tidy?rev=266183&r1=266182&r2=266183&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/.clang-tidy (original)
>>>> +++ llvm/trunk/.clang-tidy Wed Apr 13 03:58:52 2016
>>>> @@ -1,4 +1,4 @@
>>>> -Checks:
>>>> '-*,clang-diagnostic-*,llvm-*,misc-*,readability-identifier-naming'
>>>> +Checks:
>>>> '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,readability-identifier-naming'
>>>>  CheckOptions:
>>>>    - key:             readability-identifier-naming.ClassCase
>>>>      value:           CamelCase
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160422/a2ebfd6c/attachment.html>