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

[Argyrios Kyrtzidis]

On Nov 10, 2010, at 10:55 PM, Chris Lattner wrote:

> 
> On Nov 10, 2010, at 7: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.
>> 
>> 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
> 
> Are these timings with assertions disabled?  0.6% isn't a showstopper, but it is unfortunate.

Yep, assertions disabled.

> 
> 
> A couple random thoughts:
> 
> +  struct DiagStatePoint {
> +    DiagState *State;
> +    FullSourceLoc Loc;
> 
> This shouldn't have to store a FullSourceLoc in the vector, it can just store a SourceLocation and store the SourceMgr outside the vector.  Likewise, GetDiagStatePointForLoc, getDiagnosticLevel etc can just take a SourceLocation.  Since we're storing more per-translation unit stuff in the diagnostics object, we should really just give up and give Diagnostics a SourceManager& that is initialized at construction time.

I originally tried to do that but it wasn't appropriate due to the Diagnostic object being generally created ahead of everything else. But...

> 
> A more general concern I have about this change (pointed out by this) is that you're adding translation unit state to the Diagnostic object.  I guess we already had that due to mutation before so this isn't any worse, it just seems weird to me :)

That is a good point! How about moving that stuff out into a new DiagnosticState object that is owned by Preprocessor and gets passed to Diagnostic which will have no translation unit state anymore ?
DiagnosticState will have a SourceManager& and accept SourceLocations, not FullSourceLocs.

> 
> +  bool isBeforeThan(SourceLocation Loc) const;
> +  bool isBeforeThan(const FullSourceLoc &Loc) const {
> 
> The name 'isBeforeThan' doesn't make a lot of sense to me.  Also, please add a doxygen comment.  Why does this method pass FullSourceLoc by const& when the others pass by value?

This won't be necessary with the above suggestion.

-Argiris

> 
> Otherwise, looks great, thanks for working on this!!
> 
> -Chris
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits