[PATCH] Change the behavior of clang-tidy -checks=, remove -disable-checks.

[Alexander Kornienko]

On Thu, May 15, 2014 at 10:34 AM, Manuel Klimek <klimek at google.com> wrote:

> On Thu, May 15, 2014 at 10:09 AM, Alexander Kornienko <alexfh at google.com>wrote:
>
>>
>> On 15 May 2014 10:03, "Manuel Klimek" <klimek at google.com> wrote:
>> >
>> > After thinking a bit about this, I'd like to revisit this from scratch
>> (as it has changed quite a bit).
>> >
>> > Why is: -check=<regexp> not enough? Give it a good enough default value
>> (that is printed if you say -help), and we have the following use cases:
>> > - add something to the default set: clang-tidy -help, copy default set,
>> add own
>>
>> This is already problematic: making the user copy default value and
>> adjust it is inconvenient.
>>
> On the other hand, I expect it to take less time for a user to copy and
> adjust a simple regexp than to understand how the list of regexps is built
> up.
>

It's a list of globs, regexps seem to be an overkill for this purpose. It
could be a list of prefixes, but globs seem to be more intuitive.


> I'd say that having to figure out and remember that you need to add "-.*"
> in front of your checks is more inconvenient than to do -help and copy a
> regexp.
>

The problem with current approach is that we have -checks= and
-disable-checks= as two different flags, which are applied in a certain
order and don't inherit default values. This has many implications:
  * it's impossible to just re-enable some of the disabled checks (e.g.,
you want to run normal set of checks + a single experimental check, which
is disabled by default)
  * it's difficult to disable one check: just specifying
-disable-checks=one-noisy-check resets the default value of
-disable-checks=, which could contain
'many-buggy-checks|many-noisy-checks'. This is far from being convenient
and is rarely what the user wants.

So I suggest this behavior as a more convenient and flexible alternative.


> One question is: what's the more common use case?
> I'd expect this:
> - people will configure the checks they want to run in general for their
> project once
> - people will want to run a single check to see what it triggers, if it's
> fine, they'll add it to their configuration
>

There are more use-cases to consider:
  * convenience of the command-line interface: first-time
users/experimenters may want to conveniently try different checks and their
combinations without creating/editing configuration files - this needs the
ability to adjust default settings without completely resetting them.
  * discoverability of new checks: regular users, who want to automatically
get all new checks, and disable only the ones they don't like/need - this
needs wildcards and negative filters.


>
> Do you expect users will change the checks they use a lot?
>
>> Also, how do you exclude something? Say, you run clang-tidy and see a
>> number of results from checks you don't care about. How do you exclude them?
>>
> I figure out which checks I care about and include them.
>

It's inconvenient when the list of checks is large.


>
>
>>  > - run only a single check: -check=my-full.check
>> > - run only google checks: -check=google-.*
>> >
>> >
>> >
>> >
>> >
>> > On Wed, May 14, 2014 at 10:13 PM, Alexander Kornienko <
>> alexfh at google.com> wrote:
>> >>
>> >> Hi klimek,
>> >>
>> >> Make checks filtering more intuitive and easy to use. Remove
>> >> -disable-checks and change the format of -checks= to a comma-separated
>> list of
>> >> globs with optional '-' prefix to denote exclusion. The -checks=
>> option is now
>> >> cumulative, so it modifies defaults, not overrides them. Each glob
>> adds or
>> >> removes to the current set of checks, so the filter can be refined or
>> overriden
>> >> by adding globs.
>> >>
>> >> Example:
>> >>   The default value for -checks= is
>> >>
>> '*,-clang-analyzer-alpha*,-llvm-include-order,-llvm-namespace-comment,-google-*',
>> >>   which allows all checks except for the ones named
>> clang-analyzer-alpha* and
>> >>   others specified with the leading '-'. To allow all google-* checks
>> one can
>> >>   write:
>> >>     clang-tidy -checks=google-* ...
>> >>   If one needs only google-* checks, we first need to remove
>> everything (-*):
>> >>     clang-tidy -checks=-*,google-*
>> >>   etc.
>> >>
>> >> I'm not sure if we need to change something here, so I didn't touch
>> the docs
>> >> yet.
>> >>
>> >> http://reviews.llvm.org/D3770
>> >>
>> >> Files:
>> >>   clang-tidy/ClangTidyDiagnosticConsumer.cpp
>> >>   clang-tidy/ClangTidyDiagnosticConsumer.h
>> >>   clang-tidy/ClangTidyOptions.h
>> >>   clang-tidy/tool/ClangTidyMain.cpp
>> >>   test/clang-tidy/arg-comments.cpp
>> >>   test/clang-tidy/basic.cpp
>> >>   test/clang-tidy/check_clang_tidy_fix.sh
>> >>   test/clang-tidy/check_clang_tidy_output.sh
>> >>   test/clang-tidy/deduplication.cpp
>> >>   test/clang-tidy/diagnostic.cpp
>> >>   test/clang-tidy/file-filter.cpp
>> >>   test/clang-tidy/fix.cpp
>> >>   test/clang-tidy/macros.cpp
>> >>   test/clang-tidy/nolint.cpp
>> >>   test/clang-tidy/select-checks.cpp
>> >>   test/clang-tidy/static-analyzer.cpp
>> >>   test/clang-tidy/temporaries.cpp
>> >>   unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
>> >
>> >
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140515/ef0f146b/attachment.html>