[cfe-commits] [PATCH] Fix broken diagnostic pragmas

[Douglas Gregor]

On Nov 11, 2010, at 12:44 PM, Argyrios Kyrtzidis wrote:

> 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.

Sounds good, thanks!

> This also improves a bit the current situation where we deserialize all macros if the diagnostic is enabled.

Yes!

	- Doug