Page MenuHomePhabricator

[libclang] Allow skipping warnings from all included files
Needs ReviewPublic

Authored by yvvan on Jul 25 2018, 6:08 AM.

Details

Summary

Updated clone for D48116 by Nikolai, now also adds the clang driver flag and the test for it.
If clang has plugins they are also affected by this filtering.

Depending on the included files and the used warning flags, e.g. -
Weverything, a huge number of warnings can be reported for included
files. As processing that many diagnostics comes with a performance
impact and not all clients are interested in those diagnostics, add a
flag to skip them.

Diff Detail

Event Timeline

yvvan created this revision.Jul 25 2018, 6:08 AM
yvvan updated this revision to Diff 158211.Jul 31 2018, 4:05 AM

Restore missing tests

ilya-biryukov added inline comments.Jul 31 2018, 4:38 AM
include/clang/Driver/Options.td
1745

Why would we ever want to suppress diagnostics from *all* included files in the compiler mode?

That seems more appropriate in the tooling mode (e.g. IDE integration, etc.). However, that could probably be done without modifications of core clang bits, e.g. by writing a DiagnosticsConsumer that ignores diagnostics coming from outside the main file.

yvvan added a comment.Jul 31 2018, 4:48 AM

I already mentioned in the review inherited by this one that this way covers also loaded plugins with different consumers. So let's assume that driver loads clang-tidy and some other plugins and runs over each file in the project.
It makes sense not to include clang-tidy diagnostics coming from headers, at least for third-party headers (you can still run clang over specific headers themselves to get diagnostics for them).

I already mentioned in the review inherited by this one that this way covers also loaded plugins with different consumers. So let's assume that driver loads clang-tidy and some other plugins and runs over each file in the project.
It makes sense not to include clang-tidy diagnostics coming from headers, at least for third-party headers (you can still run clang over specific headers themselves to get diagnostics for them).

For third-party headers, sure (as I mentioned before, we already use -isystem include directive for third party libs to achieve the same thing). But ignoring diagnostics from all the headers does not seem particularly useful.

yvvan added a comment.Jul 31 2018, 6:10 AM

And we already saw, that -isystem is not the best choice for that. And by the way - clang-tidy has this filtering in consumer which does not exist in it's plugin-mode so it spits all system diagnostics all the time...

yvvan added a comment.Jul 31 2018, 6:20 AM

Anyways, if c++ part does not seem relevant I'm fine if we only have libclang part from D48116.

BTW I don't remember if you answered something to my suggestion of adding flag -ithird-party as an alternative to -isystem which does not lock files and helps to filter diagnostics, etc.

And we already saw, that -isystem is not the best choice for that.

Are you referring to the file-locking on Windows?
Any other reasons why the -isystem trick might be non-ideal?

And by the way - clang-tidy has this filtering in consumer which does not exist in it's plugin-mode so it spits all system diagnostics all the time...

I would still argue against adding this option in the current form to the compiler. I don't see how that might be useful to non-clang-tidy, non-editor-integration users.

What's the 'plugin mode' of clang-tidy? I don't seem to able to Google it :-)

yvvan added a comment.Jul 31 2018, 6:50 AM

And we already saw, that -isystem is not the best choice for that.

Are you referring to the file-locking on Windows?
Any other reasons why the -isystem trick might be non-ideal?

File locking is the first one. Another one comes with "plugin mode" of tidy.

And by the way - clang-tidy has this filtering in consumer which does not exist in it's plugin-mode so it spits all system diagnostics all the time...

I would still argue against adding this option in the current form to the compiler. I don't see how that might be useful to non-clang-tidy, non-editor-integration users.

What's the 'plugin mode' of clang-tidy? I don't seem to able to Google it :-)

Clang tidy is not only a standalone tool but also a plugin. It's almost never used this way (but we do that in Qt Creator to combine it with Clazy) and it also seems that some of the recent checks are added only to standalone tool (which is easy to fix).
Being loaded into clang as plugin it does not support most of clang-tidy command line options and is not able to filter system includes. I'm not sure it's possible to fix both of these issues (and probably some other).

Clang tidy is not only a standalone tool but also a plugin. It's almost never used this way (but we do that in Qt Creator to combine it with Clazy) and it also seems that some of the recent checks are added only to standalone tool (which is easy to fix).
Being loaded into clang as plugin it does not support most of clang-tidy command line options and is not able to filter system includes. I'm not sure it's possible to fix both of these issues (and probably some other).

Oh, I see, thanks for the pointers. Not related to this discussion, it seems reasonable to allow all the same options to be passed into clang-tidy plugin, not sure if that's easy or hard in principle to fix it.

The added option seems like a workaround for a bug in clang-tidy and a windows locking bug (which is blocker for tools like clangd and libclang-based integrations, IMO). I would still vouch for fixing the bugs than adding flags that try to workaround those bugs.