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