Page MenuHomePhabricator

[clang-tidy] Ignore diagnostics due to macro expansion from not-interested headers
Needs ReviewPublic

Authored by DmitryPolukhin on Nov 5 2020, 3:23 AM.

Details

Summary

This diff is an attempt to workaround a common problem with clang-tidy deployment.
There are headers that might not be compatible with your project style guide but you
have to use macro from that headers so clang-tidy could report lots of style issues
that you cannot fix or is some cases even annotate with NOLINT. Solution proposed
in this diff is to avoid reporting diagnostics in macro expansion of the macro defined
in headers that doesn't match the desired --header-filter.

Just one real life example of this issue from a very popular gflags library.
This library requires using macro in use code like this:

DEFINE_bool(some_bool_flag,
            "the default value should go here, not the description",
            false);

But the macro has outdated version of compiler assert that uses C-style arrays.
Therefore use code becomes incompatible with clang-tidy check modernize-avoid-c-arrays.
Another example of problematic is googletest/googlemock with lots of macro that you cannot avoid.

New feature implemented behind configuration flag for backward compatibility.

Diff Detail

Event Timeline

DmitryPolukhin created this revision.Nov 5 2020, 3:23 AM
DmitryPolukhin requested review of this revision.Nov 5 2020, 3:23 AM

Fix clang-tidy warning

njames93 added inline comments.Nov 5 2020, 12:36 PM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
280–282

This should also check if the Optional has a value

382–384

This looks suspicious, in clang-tidy land SystemHeaders is always set, however outside clang-tidy it may not be.
Perhaps getValueOr(false) should be used to prevent any asserts.

Also can a test be added to show this respecting the SystemHeaders setting.

391–393

Elide braces

DmitryPolukhin marked 2 inline comments as done.
  • addressed comments in the diff
  • stop ignoring system macro

@njames93 thank you for the feedback!

Still waiting for more feedback and especially high level one about this feature in general. Does it look useful? Should it be behind a flag i.e. changing default behaviour is too dangerous?

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
280–282

This code was moved from ClangTidyDiagnosticConsumer::getHeaderFilter but you are right, it makes sense to add support for missing value.

382–384

Running this code on bigger codebase I found undesired side-effect that it is better to report issues on system macro like NULL such issues usually from external code, not from macro itself.

zigwei added a subscriber: zigwei.EditedApr 5 2021, 4:47 PM

This is a helpful feature. We had the same problems with both GFLAG and GTEST, too.
But I am not sure if HeaderFilterRegex is the right option to control it. Is a new filter better for the ignored headers?

antonl added a subscriber: antonl.Apr 5 2021, 6:18 PM

Let me second that this will be a very useful feature. Without enabling some clang-tidy checks will be very noisy.

I think there is no sense to invent another HeaderFilterRegex like option to control which headers should be excluded. HeaderFilterRegex should be good enough because if you would like to see warning from the header, most probably you can fix code in the header or at least add a suppression.

To be on a safe side I'm going to put this feature behind a configuration flag and send diff for review soon.

Put feature behind flag

Fix wrong character

DmitryPolukhin retitled this revision from [RFC][clang-tidy] Ignore diagnostics due to macro expansion from not-interested headers to [clang-tidy] Ignore diagnostics due to macro expansion from not-interested headers.Apr 19 2021, 7:33 AM
DmitryPolukhin edited the summary of this revision. (Show Details)

Added test for system like object macro

@alexfh, @njames93 and @thakis please take a look! I added all tests cases and put new logic behind a flag to make it as safe as possible.
Issue with diagnostics from macro expansion from third-party headers is the one of the biggest problem with deployment that we have and it cannot be properly fixed with wrappers around clang-tidy.

This needs to explain why the existing functionality isn't sufficient - if the header is really not from this project,
then it should be included via -isystem, and that will naturally suppress all diagnostics from it.

This needs to explain why the existing functionality isn't sufficient - if the header is really not from this project,
then it should be included via -isystem, and that will naturally suppress all diagnostics from it.

The diagnostics are reported in the place of macro expansion i.e. user sources. It doesn't matter how they get included via -isystem or -I.