[clang-tidy] Excluding individual files/lines from clang-tidy checks

Hi,

I want to implement a mechanism to exclude individual files/lines from
clang-tidy checks. The existing '-line-filter' option isn't
particularly suitable for this purpose since it requires listing all
checked files along with line ranges. Furthermore, this functionality
was already requested (or at least mentioned) in this bug -
https://llvm.org/bugs/show_bug.cgi?id=25215 I believe it might be
useful to a broader audience as well.

I need advice on the interface of this mechanism. Other clan-tidy
options (like '-checks' or '-warnings-as-errors') use the minus sign
to denote an exclusion. However, this approach doesn't play well with
the JSON format of the '-line-filter' string. So I'm thinking about
adding another option - '-exclude-from-checks' - that will take a JSON
array identical to the one '-line-filter' takes:

clang-tidy '-exclude-from-checks=[{"name":"exclude.c","lines":[[5,7]]}]' ...

I can process this option after the line filter. This way
'-exclude-from-checks' could be used together with both
'-header-filter' and '-line-filter'. Doing this will also maintain
backward-compatibility.

I would appreciate any suggestions or comments on my idea.

Regards,
Nikita

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAGJfN2yb=3D-oCyTycsULN7F=Gn=KxFW_O4r4ndzCd8bFZ2muw@mail.gmail.com>,
    Nikita via cfe-dev <cfe-dev@lists.llvm.org> writes:

> clang-tidy '-exclude-from-checks=[{"name":"exclude.c","lines":[[5,7]]}]'

Why are we specifying command-line arguments as JSON?

It seems unnecessarily cumbersome to me.

Well, I'm trying to mimic the behavior of '-line-filter'. But I also
think that JSON is cumbersome. It requires putting arguments in
quotes, plus the format itself seems a bit redundant. Specifying
'[{"name":"file.h"}]' without the line range seemingly does nothing.
Diagnostics for "file.h" will be emitted even without this filter.

I think I'll explain why I need this functionality and why I think it
would be useful for others.

Currently clang-tidy doesn't provide a practical way to suppress
diagnostics from headers you don't have control over. If using a
function defined in such header causes a false positive, there is no
easy way to deal with this issue. Since you cannot modify a header,
you cannot add NOLINT to it. And adding NOLINT to each invocation of
such function is either impossible or hugely impractical if
invocations are ubiquitous.

Using the '-header-filter' doesn't help to address this issue as well.
Unless your own headers are strategically named, it is virtually
impossible to craft a regex that will filter out only the third-party
ones.

The '-line-filter' can be used to suppress such warnings, but it's not
very convenient. Instead of excluding a line that produces a warning
you have to include all other lines. Plus, it provides no way to
suppress only certain warnings from a file.

With all this mentioned, I think that clang-tidy would benefit from a
mechanism that is specifically designed to suppress certain warnings
from certain lines in a convenient way.

I don't see how existing options can be changed to provide this
functionality, so I propose adding a new option -
'--exclude-from-checks' - that will take a list of locations to
suppress warnings from. It can additionally take a list of checks to
suppress. The format of its' argument can be a JSON array similar to
the one '-line-filter' takes:
[{"name":"header.h","lines":[[1,5]],"checks":"cert-*"}].

I would gladly appreciate any feedback on this idea and look forward
to the discussion.

Regards,
Nikita

That causes some maintainability issues though: let say I filter something between line 1000 and 1005 today, what if some code is later added before?
Do you see a way of handling this?

No, currently I cannot see a reasonable way to handle this situation.
Probably, line ranges are unnecessary. But suppressing certain checks
from entire files is still useful for filtering out false positives.

I can omit line ranges and make '--suppress-warnings-from' take only
file names and checks: [{"name":"header.h","checks":"cert-*"}].

Does this look good now?

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <A04C45C0-2B94-4584-9661-4B2E39CA65D4@apple.com>,
    Mehdi Amini via cfe-dev <cfe-dev@lists.llvm.org> writes:

That causes some maintainability issues though: let say I filter something
between line 1000 and 1005 today, what if some code is later added before?

This is why with static analysis tools most people prefer marking
exceptions with a comment that follows the code around instead of
specifying a fragile file/line combination on the command line or in
a configuration file.

Do you see a way of handling this?

Either use some sort of source annotation like the comment above or
identify the chunk of code with some sort of universal address that
drills down by some combination of name and block identifiers. Even
those such addresses can change under maintenance, however.

use some sort of source annotation

This approach works well for sources I have control over. But trying
to apply it to third-party headers will result in a significant
maintenance burden.

The option I propose targets third-party headers specifically. The
goal is to be able to exclude certain third-party headers from checks
and thus avoid maintaining patches with source annotations.

By the way, with line ranges omitted it is easier to ditch JSON and go
for a simpler format.
Like this: 'header1.h:cert-*,boost-*;header2.h:clang-analyzer-*'.
Less cumbersome, more comfortable to work with in, say, CMake files.

I've submitted a patch adding this functionality a couple of weeks ago
- https://reviews.llvm.org/D26418
But the review is stuck - @alexfh hasn't been online for two weeks.

Could someone please review this patch?

Regards,
Nikita