On Nov 10, 2010, at 9:34 PM, Argyrios Kyrtzidis wrote:
> Diagnostic pragmas are broken because we don't keep track of the diagnostic
> state changes and we only check the current/latest state.
> Problems manifest if a diagnostic is emitted for a source line that has
> different diagnostic state than the current state; this can affect
> a lot of places, like C++ inline methods, template instantiations, the lexer,
> etc. For example:
>
> struct S {
> #pragma clang diagnostic push
> #pragma clang diagnostic ignored "-Wtautological-compare"
> void m() { int b = b==b; }
> #pragma clang diagnostic pop
> };
>
> the pragmas do not work for suppressing the warning in the above case.
>
> The attached patch fixes the issue by having the Diagnostic object keep track
> of the source location of the pragmas so that it is able to know what is the
> diagnostic state at any given source location.
This is really cool, Argiris!
@@ -365,33 +376,56 @@ void Diagnostic::ReportDelayed() {
DelayedDiagArg2.clear();
}
+Diagnostic::DiagStatePointsTy::iterator
+Diagnostic::GetDiagStatePointForLoc(FullSourceLoc Loc) const {
+ assert(!DiagStatePoints.empty());
+ assert(DiagStatePoints.front().Loc.isInvalid() &&
+ "Should have created a DiagStatePoint for command-line");
+
+ if (Loc.isInvalid())
+ return DiagStatePoints.begin();
I guess I was expecting that we would return the last DiagStatePoint for an
invalid location (so that we get the most recent state), rather than the first.
diff --git a/lib/Lex/PPLexerChange.cpp b/lib/Lex/PPLexerChange.cpp
index 4a40405..51b6dae 100644
--- a/lib/Lex/PPLexerChange.cpp
+++ b/lib/Lex/PPLexerChange.cpp
@@ -252,12 +252,14 @@ bool Preprocessor::HandleEndOfFile(Token &Result, bool
isEndOfMacro) {
// This is the end of the top-level file. If the diag::pp_macro_not_used
// diagnostic is enabled, look for macros that have not been used.
- if (getDiagnostics().getDiagnosticLevel(diag::pp_macro_not_used) !=
- Diagnostic::Ignored) {
- for (macro_iterator I = macro_begin(false), E = macro_end(false);
- I != E; ++I)
- if (!I->second->isUsed())
- Diag(I->second->getDefinitionLoc(), diag::pp_macro_not_used);
+ for (macro_iterator I = macro_begin(false), E = macro_end(false);
+ I != E; ++I) {
+ if (I->second->isUsed())
+ continue;
+ FullSourceLoc DefLoc(I->second->getDefinitionLoc(), getSourceManager());
+ if (getDiagnostics().getDiagnosticLevel(diag::pp_macro_not_used, DefLoc) !=
+ Diagnostic::Ignored)
+ Diag(DefLoc, diag::pp_macro_not_used);
}
It's unfortunate this fixing this check means that we always have to iterate
over all of the macros. In particular, I wonder if this causes deserialization
of macros of a PCH file, which would be a performance killer. Could you check?
> I've also made measurements to see how this fix impacts performance. I
> basically measured how long it takes to do a -fsyntax-only over the
> llvm/clang codebase and the results were:
>
> before the fix: 722.83 sec
> after the fix: 727.24 sec
>
> So avoiding the fix only provides a 0.6% gain.
Other than the above, I didn't notice any other opportunities for optimization
here. I think we have to suck up the cost in the name of correctness.
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
