Page MenuHomePhabricator

[libclang] Allow skipping warnings from all included files
ClosedPublic

Authored by nik on Jun 13 2018, 12:44 AM.

Details

Summary

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

nik created this revision.Jun 13 2018, 12:44 AM
yvvan added a comment.Jun 13 2018, 1:49 AM

Cool! That's actually quite a small change with big outcome!

include/clang/Basic/Diagnostic.h
216

Probably mention that it also suppresses everything with lower priority?

lib/Basic/DiagnosticIDs.cpp
484

{ } around return

nik marked 2 inline comments as done.Jun 13 2018, 2:49 AM
nik updated this revision to Diff 151116.Jun 13 2018, 2:50 AM

Addressed comments.

yvvan added a comment.Jun 21 2018, 2:01 AM

@ilya-biryukov what do you think?

Sorry for the late response, was on vacation.

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

I wonder if you want to handle notes and remarks in a special manner? They can be seen as part of the original diagnostic, rather than the separate diagnostic. E.g. showing a note in the main file, but not showing the diagnostic from the headers file that this note comes from, might be confusing to the users.
Maybe also add tests for diagnostics in the main file with notes/remarks in the header files and vice versa?

yvvan added a comment.Jun 27 2018, 4:28 AM

The idea was to ignore everything including notes and remarks so that only errors from system headers are still collected.

Changing consumer might be a possible way to go as well but it requires changing (or wrapping) all consumers that we need to be affected. The advantage of this change that it also covers consumers that come from plugins that we use (tidy and clazy).

yvvan added a comment.Jun 27 2018, 5:22 AM

But this one misses a way to set this flag for everything except libclang. We can probably change that (Nikolai is in vacation till the end of summer so it's probably my part now) and add some tests outside of Index for it (probably Frontend)

nik added a comment.Dec 6 2018, 8:35 AM

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Will check.

I wonder if you want to handle notes and remarks in a special manner? They can be seen as part of the original diagnostic, rather than the separate diagnostic. E.g. showing a note in the main file, but not showing the diagnostic from the headers file that this note comes from, might be confusing to the users.

Looks like this case is handled fine.

WIthout the new option, a note is printed as expected/always (shortened output):

$ bin/c-index-test -test-load-source function ignore-warnings-from-headers.cpp -Wunused-parameter
ignore-warnings-from-headers.h:1:12: warning: unused parameter 'unusedInHeader' [-Wunused-parameter]
ignore-warnings-from-headers.cpp:1:10: note: in file included from ignore-warnings-from-headers.cpp:1:
ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 'unusedInMainFile' [-Wunused-parameter]

With the new option, note is not printed and thus no confusion should arise:

$ CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES=1 bin/c-index-test -test-load-source function ignore-warnings-from-headers.cpp -Wunused-parameter
ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 'unusedInMainFile' [-Wunused-parameter]

I will add "// CHECK-NOT: note: in file included from" to the test.

Maybe also add tests for diagnostics in the main file with notes/remarks in the header files and vice versa?

"Vice versa" is covered as shown above. If the diagnostic is in main file and a note of that one refers to the header, then the note should be shown/included. This case seems also fine - I've added a test for this.

But this one misses a way to set this flag for everything except libclang. We can probably change that (Nikolai is in vacation till the end of summer so it's probably my part now) and add some tests outside of Index for it (probably Frontend)

What's the use case of this?

nik added a comment.Dec 7 2018, 4:04 AM

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to filter things.

For filtering in StoredDiagnosticConsumer one needs to pass the new bool everywhere along where "bool CaptureDiagnostics" is already passed on (to end up in the StoredDiagnosticConsumer constructor) . Also, ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) convert "bool CaptureDiagnostics" to an enum with enumerators like CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this a bit less invasive.

If changing clang's diagnostic interface should be avoided, I tend to go with (2). Ilya?

$ git grep "bool CaptureDiagnostics"
include/clang/Frontend/ASTUnit.h:  bool CaptureDiagnostics = false;
include/clang/Frontend/ASTUnit.h:                             ASTUnit &AST, bool CaptureDiagnostics,
include/clang/Frontend/ASTUnit.h:         IntrusiveRefCntPtr<DiagnosticsEngine> Diags, bool CaptureDiagnostics,
include/clang/Frontend/ASTUnit.h:      bool CaptureDiagnostics = false, bool AllowPCHWithCompilerErrors = false,
include/clang/Frontend/ASTUnit.h:      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
include/clang/Frontend/ASTUnit.h:      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
include/clang/Frontend/ASTUnit.h:      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
lib/Frontend/ASTUnit.cpp:                             ASTUnit &AST, bool CaptureDiagnostics,
lib/Frontend/ASTUnit.cpp:    bool CaptureDiagnostics, bool AllowPCHWithCompilerErrors,
lib/Frontend/ASTUnit.cpp:                bool CaptureDiagnostics, bool UserFilesAreVolatile) {
lib/Frontend/ASTUnit.cpp:    bool OnlyLocalDecls, bool CaptureDiagnostics,
lib/Frontend/ASTUnit.cpp:    bool OnlyLocalDecls, bool CaptureDiagnostics,
lib/Frontend/ASTUnit.cpp:    bool OnlyLocalDecls, bool CaptureDiagnostics,
tools/libclang/Indexing.cpp:  bool CaptureDiagnostics = !Logger::isLoggingEnabled();
In D48116#1322878, @nik wrote:

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to filter things.

I

For filtering in StoredDiagnosticConsumer one needs to pass the new bool everywhere along where "bool CaptureDiagnostics" is already passed on (to end up in the StoredDiagnosticConsumer constructor) . Also, ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) convert "bool CaptureDiagnostics" to an enum with enumerators like CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this a bit less invasive.
If changing clang's diagnostic interface should be avoided, I tend to go with (2). Ilya?

Yeah, LG. The changes in the ASTUnit look strictly better than changes in clang - the latter seems to already provide enough to do the filtering.
If you avoid changing the StoredDiagnosticConsumer (or writing a filtering wrapper for DiagnosticConsumer), you'll end up having some diagnostics inside headers generated after preamble was built, right?
That might be either ok or not, depending on the use-case.

nik added a comment.Feb 19 2019, 12:06 AM

For filtering in StoredDiagnosticConsumer one needs to pass the new bool everywhere along where "bool CaptureDiagnostics" is already passed on (to end up in the StoredDiagnosticConsumer constructor) . Also, ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) convert "bool CaptureDiagnostics" to an enum with enumerators like CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this a bit less invasive.
If changing clang's diagnostic interface should be avoided, I tend to go with (2). Ilya?

Yeah, LG. The changes in the ASTUnit look strictly better than changes in clang - the latter seems to already provide enough to do the filtering.
If you avoid changing the StoredDiagnosticConsumer (or writing a filtering wrapper for DiagnosticConsumer), you'll end up having some diagnostics inside headers generated after preamble was built, right?

If there is some #include after the first declaration, possibly. Why is that relevant?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 12:06 AM
nik updated this revision to Diff 187318.Feb 19 2019, 12:07 AM

OK, filtering happens now in FilterAndStoreDiagnosticConsumer, the former
StoredDiagnosticConsumer.

This LGTM wrt to layering, i.e. it's nice we don't need to change the code of diagnostics.
It's been a while since I've looked at the ASTUnit code, though, would be good if someone else took an extra look at it.

Added a few nits too.

lib/Frontend/ASTUnit.cpp
682 ↗(On Diff #187318)

isWrittenInMainFile might be a better fit: it does not look at presumed locations. That would be the expected behavior in the most common case, i.e. showing errors in an IDE or a text editor.

694 ↗(On Diff #187318)

Why do we need this extra check? We checked for StoredDiags and Standalone diags in the body of the statement later again.

nik marked 4 inline comments as done.May 10 2019, 7:26 AM
nik added inline comments.
lib/Frontend/ASTUnit.cpp
682 ↗(On Diff #187318)

Oh, good catch! Thanks! :)

694 ↗(On Diff #187318)

Ops, looks like a left-over from a previous version, removed.

nik updated this revision to Diff 199015.May 10 2019, 7:26 AM
nik marked 2 inline comments as done.

Addressed inline comments.

nik added a comment.May 10 2019, 7:27 AM

Sorry for the pointless ping, haven't seen the inline comments. They are addressed now.

I've also increased CINDEX_VERSION_MINOR so clients can detect availability of this new flag.

It's been a while since I've looked at the ASTUnit code, though, would be good if someone else took an extra look at it.

Benjamin? Argyrios? Would you be so kind? :)

@jkorous, could you take a look or suggest someone who can review the libclang changes?
As mentioned before, I've reviewed the clang bits (which are now gone), but don't have much experience in libclang parts.

yvvan accepted this revision.Jun 5 2019, 4:02 AM

libclang part is quite small here and looks ok. I would just accept it

This revision is now accepted and ready to land.Jun 5 2019, 4:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 7:12 AM