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

Fri Apr 22 13:09:03 PDT 2016

On Fri, Apr 22, 2016 at 12:14 PM, Alexander Kornienko <alexfh at google.com>
wrote:

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

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)

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

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

>
>
>> 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/cdbeabbb/attachment.html>