[cfe-commits] [Patch 2 of 4] -verify fixes (stage 2)

[Jordan Rose]

On Jul 26, 2012, at 2:14 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Wednesday, July 25, 2012 4:49 PM, Argyrios Kyrtzidis wrote:
>> Correct me if I'm wrong but, from what I understand, the original issue
>> that motivated Andy's patch is that the passed in DiagnosticConsumer
>> (which can be a VerifyDiagnosticConsumer) is not "initialized" properly
>> by calling BeginSourceFile on it before parsing starts (thus the
>> VerifyDiagnosticConsumer cannot hook in the Preprocessor and receive the
>> comments).
>> 
>> Why not just adding a BeginSourceFile in CaptureDiagnosticConsumer which
>> will just call BeginSourceFile on the DiagnosticConsumer that was passed
>> in to arcmt::checkForManualIssues ?
>> 
>> That way the "contract" of the DiagnosticConsumer (call BeginSourceFile
>> with the preprocessor before parsing is initiated) will be respected and
>> VerifyDiagnosticConsumer should be able to get the comments without
>> arcmt::checkForManualIssues needing to worry about it.
> 
> You know, that was just too blindingly obvious for me to think of!!  No,
> seriously, of course this is the way to go and attached is a patch that
> does just that.  Thanks for pulling me out of that dead end!
> 
> A note regarding the implementation:
> 
> It is not possible to simply forward BeginSourceFile/EndSourceFile calls
> as they occur, since this will cause VerifyDiagnosticConsumer to try to
> verify diagnostics without diagnostics being forwarded.  This means that
> the first BeginSourceFile call is forwarded (this is all that is required
> to set up the preprocessor hooks) and the matching EndSourceFile is then
> called after the diagnostics list has been processed.  This is done by
> an explicit call to a new method "FinishCapture" (it cannot be done
> automatically in the destructor of CaptureDiagnosticConsumer since by
> then the Preprocessor instance has become invalid).

I see. I realize that it's slightly less safe, but is it viable to call FinishCapture in the destructor of CaptureDiagnosticsConsumer? That way there's no ugly explicit cleanup calls in the user. (I realize there are potential problems with that, but it seems a lot cleaner!)

If we can't do that, please do keep the assertion.


> This also necessitated a change to VerifyDiagnosticConsumer so that it
> only processes the directives list once the final EndSourceFile call is
> reached (otherwise, again, it will process them too early).

I'm still confused by this (pending discussion on patch 1 thread).

> Much better now, yes?  :o)

Yes! Thanks, Andy; thanks, Argyrios.

Jordan