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.
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.
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 :)
+ 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?
Otherwise, looks great, thanks for working on this!!
-Chris
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
