On Nov 10, 2010, at 10:06 PM, Douglas Gregor wrote:
>
> 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.
Yes, that is more appropriate, I'll make some changes.
>
> 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?
That is badness.
How about this; Since we know at macro creation time whether it needs to be
checked or not, keep that information (CheckUsed) in MacroInfo.
For de/serialization keep in PCH the set of macros that need checking
(CheckUsed && !IsUsed) and only deserialize these macros.
This also improves a bit the current situation where we deserialize all macros
if the diagnostic is enabled.
-Argiris
>
>> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
