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

[Jordan Rose]

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

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