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

[Argyrios Kyrtzidis]

Hey Andy & Jordan,

On Jul 25, 2012, at 12:38 AM, Jordan Rose wrote:

> 
> On Jul 24, 2012, at 13:58 , Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
> 
>> On Tuesday, July 24, 2012 8:05 PM, Jordan Rose wrote:
>> 
>>> I still think this is a hack. I don't think DiagnosticConsumers /should/
>>> have isa/cast/dyn_cast. The whole point of OOP and an open class hierarchy
>>> is that you use polymorphism as much as possible.

I agree with Jordan that adding dyn_cast on DiagnosticConsumers is not a good idea in general.
And the particular use of it in the patch is more of a hack than a good argument to support dyn_cast.

>> 
>> This was the essence of my first approach and the implementation of a
>> virtual getCommentHandler() function but this was (in my opinion) rightly
>> rejected by my reviewers as a horrible hack.  :o)
>> 
>>> Is it possible to just add a "shouldCaptureDiagnostics" argument to
>>> arcmt::checkForManualIssues? I think that's really the cleanest way to
>>> solve the problem.
>> 
>> I assume you mean add "shouldCaptureDiagnostics" so that the code performs
>> a straight static_cast take DiagClient from DiagnosticConsumer* to
>> VerifyDiagnosticConsumer*.  But I really don't favour this over a proper
>> dyn_cast solution since it would only need someone in future to misuse
>> the parameter "shouldCaptureDiagnostics", for as simple a reason as a
>> misunderstanding as to its use, and for it to cause havoc!  With dyn_cast
>> such misuse *should* be avoidable by design, plus it really doesn't add
>> more than the smallest overhead in terms of code size, and next to none
>> in terms of running performance.
> 
> That's not what I meant at all. :-)
> 
> My idea is that arcmt::checkForManualIssues should simply not replace the diagnostic client if one of its arguments, shouldCaptureDiagnostics, is set to false. Whatever the existing client is will then process the diagnostics normally, as expected. No CaptureDiagnosticConsumer is created. In practice, this flag will only be used by arcmt-test.

I don't think this is right. The migrator needs to capture the diagnostics so that it can filter them and pass to the actual diagnostic consumer only those that it cannot "fix". This is true irrespective of what kind the diagnostic consumer is, e.g. if the VerifyDiagnosticConsumer received the diagnostics normally we would not be able to verify which ones were fixed.

> 
> This is NOT a setting on DiagnosticEngine, only a retrofit onto the ARC migrator, since we like using -verify there. I think saying that VerifyDC and CaptureDC are incompatible is acceptable. But maybe someone else will disagree with me there.
> 
> Jordan

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.

-Argyrios