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.
Details
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 31743 Build 31742: arc lint + arc unit
Event Timeline
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?
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).
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)
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.
What's the use case of this?
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();
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.
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?
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 | 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 | Why do we need this extra check? We checked for StoredDiags and Standalone diags in the body of the statement later again. |
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.
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.