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

Alexander Kornienko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 24 09:54:56 PST 2017


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"
  // <some code that should be clean w.r.t. check1, but may have warnings from check2>
    
  library_b/b.h:
  // <some code that is not clean w.r.t. check1, but needs to be clean from check2 warnings>

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





More information about the cfe-commits mailing list