[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

While I agree that there's an issue here that needs to be solved, I don't think 
this patch will be merged as-is -- there are technical issues brought up by 
@alexfh that have not been addressed, and I share the opinion that we should 
extend existing suppression mechanisms rather than try to invent another new 
one. If someone wanted to invest the time and energy into such a patch, I'd be 
happy to be added as a reviewer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D26418/new/

https://reviews.llvm.org/D26418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Josh via Phabricator via cfe-commits]

davidsj2 added a comment.

I too am interested in this patch.  In addition to the use cases already 
described, this would be helpful for projects that use several different static 
analysis tools and don't want to clutter code with a bunch of different 
suppression comments to satisfy each tool.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D26418/new/

https://reviews.llvm.org/D26418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Vincent Le Bourlot via Phabricator via cfe-commits]

vlebourl added a comment.

Very interested in this one as well!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D26418/new/

https://reviews.llvm.org/D26418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Lev Khoroshansky via Phabricator via cfe-commits]

khoroshansky added a comment.
Herald added a subscriber: danielkiss.

Will this ever be merged?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D26418/new/

https://reviews.llvm.org/D26418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Oleg via Phabricator via cfe-commits]

okainov added a comment.

Any updates on this one? That sounds so fundamental feature which current 
version is lacking, it's very frustrating it was not merged yet...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D26418/new/

https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[P F via cfe-commits]

> On Jan 30, 2018, at 7:50 AM, Andrew Peter Marlow via Phabricator via 
> cfe-commits  wrote:
> 
> marlowa added a comment.
> 
> In https://reviews.llvm.org/D26418#653146, @nkakuev wrote:
> 
>> Ping.
> 
> 
> another ping. I am desperate for this enhancement and am really glad that 
> nkakuev has done all the hard development work. I am currently being nobbled 
> in my use of clang-tidy because the project is using Rogue Wave Stingray 
> headers and BCG Control bar headers. Both these components cause loads of 
> clang-tidy issues when the headers get included. This enhancement seems to 
> provide just the mechanism I need.

You can already filter headers the same way you filter warnings by using 
`-isystem` flags.


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Andrew Peter Marlow via Phabricator via cfe-commits]

marlowa added a comment.

In https://reviews.llvm.org/D26418#653146, @nkakuev wrote:

> Ping.


another ping. I am desperate for this enhancement and am really glad that 
nkakuev has done all the hard development work. I am currently being nobbled in 
my use of clang-tidy because the project is using Rogue Wave Stingray headers 
and BCG Control bar headers. Both these components cause loads of clang-tidy 
issues when the headers get included. This enhancement seems to provide just 
the mechanism I need.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Adding a mechanism to supply suppression lists would be useful as long as it's 
flexible and extensible enough and doesn't significantly affect performance 
(especially, when not in use). In particular, it shouldn't be bound to a 
specific format or a specific way to store the data (there might be a default 
format and a default storage schema similar to .clang-tidy configuration files, 
but it should be easy, for example, to plug a database-backed source of 
suppression lists). Another important feature is a convenient way to add new 
suppressions. If you're willing to invest time in designing and implementing 
this, I'm happy to help with ideas and code reviews.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Nikita Kakuev via Phabricator via cfe-commits]

nkakuev added a comment.

Thanks for the response, Alex!

The problem I tried to address in this patch was that notes in source files 
were "unsuppressing" warnings from third-party headers (this is what we were 
discussing in https://reviews.llvm.org/D26418#590417).  But at this point, I no 
longer think that my patch is the right way to handle this situation. A 
general-purpose suppression list looks like a way better idea to me.

As for delegating the suppression of warnings to an external tool, I'm not 
entirely sure that this is always a good idea. I run clang-tidy on incremental 
builds on developers machines and use //-warnings-as-errors=*//. In other 
words, I treat clang-tidy warnings the same way I treat compiler warnings (with 
//-Werror// enabled). A suppression list will let me keep this workflow while 
using an external tool is likely to break it.

So what do you think about extending existing suppression mechanisms to support 
suppression lists?


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

My apologies again for the delay. There are a few things I'm concerned about:

1. Suppression list being a command-line option is going to be inconvenient to 
completely useless in many setups. The `-line-filter` option is special in this 
regard, since it was added for a rather specific use case - clang-tidy-diff.py. 
It's impractical to expect a user to specify a long blacklist on the command 
line, and it seems hacky to introduce a separate wrapper script with an 
explicit goal to pass this argument to clang-tidy.
2. There are already three mechanisms to filter warnings, introducing another 
one is going to add more complexity both in the code and in the usage of the 
tool. We should carefully consider all alternatives before doing so. In 
particular, if there are important use cases w.r.t. filtering of results 
clang-tidy doesn't handle well (but should), we could either improve existing 
suppression mechanisms or delegate warning suppression to an external tool like 
Code Checker (https://github.com/Ericsson/codechecker).

Going back to the use cases you have:

1. Header files need a different set of checks:

  library_a/a.cc:
  #include "library_b/b.h"
  // 

  library_b/b.h:
  // 

This seems like a natural use case for the `.clang-tidy` configuration file: 
one should be able to turn on `check1` in `library_a/.clang_tidy` and `check2` 
in `library_b/.clang_tidy` and expect to only get corresponding warnings in 
each of these files. The problem is that it's not implemented that way. 
Currently the configuration for the main file will be used for all diagnostics 
generated while analysing the whole translation unit. The checks filter in the 
configuration file is used for two separate purposes: creating the list of 
checks to run on each translation unit and filtering diagnostics. We can't use 
different list of checks for analyzing each header in a single translation 
unit, but it seems possible to use proper configuration for each header when 
filtering diagnostics. This way in the example above we would run `check1` for 
the whole TU, but filter out all `check1` warnings from `library_b/` files. It 
might even be possible to further improve that to create a union of sets of 
checks from all headers used by the TU and then filter out irrelevant ones, but 
it would be a different performance / precision trade-off and it doesn't seem 
to be necessary at first.

2. Header files should be clean w.r.t. a certain check, but there is a small 
number of false positives that need to be suppressed without modifying the 
source code. This seems like a use case of an external suppression list that is 
best implemented by a stateful tool like Code Checker that stores a database of 
findings and a suppression list. Alternatively, we could implement a way to 
read a suppression list from a file (command line doesn't seem to be a good 
medium for such information) that would ideally be stored and versioned with 
the code. I believe, it's the same kind of a last resort solution as `// 
NOLINT` suppression comments. It's better when each check provides a sane 
check-specific way to suppress the diagnostic (e.g. like Clang's 
`-Wparentheses` warning - https://godbolt.org/g/nVl7we).

3. Warnings in headers that are caused by notes in a different library (what 
you discuss in https://reviews.llvm.org/D26418#590417). I thought quite a bit 
about notes in user code unsuppressing warnings in third-party library code 
back in the time when implementing the corresponding clang-tidy feature, but it 
turned out to be rather infrequent situation in my setup, so I don't have a 
strong opinion on how this should be handled. Do you have more motivating 
examples showing when one would want to disable this feature? (Or am I missing 
some examples already mentioned in this thread?)


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Nikita Kakuev via Phabricator via cfe-commits]

nkakuev added a comment.

Ping.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Sorry for the delay. I'll try to get back to this patch soon.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Nikita Kakuev via cfe-commits]

nkakuev added a comment.

Ping.

Ignoring note locations is a coarse-grained solution that will suppress both 
false and //true// positives. Suppress-checks-filter is a finer-grained 
instrument that can be targetted on particular false positives precisely.

My sympathies are with suppress-checks-filter, but if it sounds like too much, 
I can try to implement note locations ignoring instead.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Malcolm Parsons via cfe-commits]

malcolm.parsons resigned from this revision.
malcolm.parsons removed a reviewer: malcolm.parsons.
malcolm.parsons added a comment.

In https://reviews.llvm.org/D26418#590476, @nkakuev wrote:

> On a second thought, ignoring note locations might be a good enough solution 
> for me.
>  How should I do this? Add a new option (e.g. '-ignore-note-locations')?


I'll let @alexfh decide.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Nikita Kakuev via cfe-commits]

nkakuev added a comment.

In https://reviews.llvm.org/D26418#590417, @malcolm.parsons wrote:

> In https://reviews.llvm.org/D26418#590383, @nkakuev wrote:
>
> > The warning is caused by a third-party code, but header filter won't help 
> > you to suppress it since it now relates to your sources.
>
>
> The header filter suppresses the warning location, but the note locations are 
> in the main file so they unsuppress the warning.
>
> It sounds like you want note locations to be ignored.


On a second thought, ignoring note locations might be a good enough solution 
for me.
How should I do this? Add a new option (e.g. '-ignore-note-locations')?


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Nikita Kakuev via cfe-commits]

nkakuev added a comment.

In https://reviews.llvm.org/D26418#590417, @malcolm.parsons wrote:

> In https://reviews.llvm.org/D26418#590383, @nkakuev wrote:
>
> > The warning is caused by a third-party code, but header filter won't help 
> > you to suppress it since it now relates to your sources.
>
>
> The header filter suppresses the warning location, but the note locations are 
> in the main file so they unsuppress the warning.
>
> It sounds like you want note locations to be ignored.


I want a convenient instrument to suppress unwanted diagnostics. Even if I 
change clang-tidy to ignore note locations, it would still be impractical to 
use '-header-filter' to //exclude// diagnostics from certain headers. Header 
filter was designed to //include// headers, not to //exclude// them. Trying to 
exclude "these m directories and that n files" is close to impossible using a 
single regular expression. Plus, even if I manage to do this, I'll be ignoring 
all diagnostics and not only the faulty ones.

Suppress-check-filter is specifically designed to //exclude// headers and 
doesn't require abusing regular expressions to do this. Plus, it allows a user 
to specify what diagnostics he doesn't want, instead of ignoring everything.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

I also don't understand the use case for turning off only some checks for 
third-party headers. Either you care about third-party stuff or not, why only 
switch off certain checks?


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26418: [clang-tidy] Add '-suppress-checks-filter' option to suppress diagnostics from certain files

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a comment.

I think will be good idea to mention this in documentation and release notes.


https://reviews.llvm.org/D26418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits