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

[Andy Gibbs]

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.

The problem is getting from DiagnosticConsumer* to VerifyDiagnosticConsumer*
by the most pain-free but also safe mechanism and I'm afraid, I'd say your
suggestion is actually slightly more of a hack than my solution, but less
of a hack than my original solution!  :o)

I'd still argue from a cleanliness and a simplicity point of view, using
dyn_cast makes most sense, since it makes automatic the link between the
hypothetical "shouldCaptureDiagnostics" and a cast from DiagnosticConsumer*
to VerifyDiagnosticConsumer*.

Going further, how should the value for "shouldCaptureDiagnostics" be
determined?  It can't be hard-coded since the -verify option may not be
used and the the DiagnosticConsumer instance may not be an instance of
VerifyDiagnosticConsumer.  If we didn't go the isa/dyn_cast route, then
I would think we would need a virtual method in DiagnosticConsumer of the
form "isVerifyDiagnosticConsumer" (or other suitable name but with the
same ultimate purpose!) but then this is no different than the route we
are trying to avoid.  If some other method is used to determine the
value of "shouldCaptureDiagnostics", would it be more reliable / more
efficient than implementing isa/dyn_cast?

What do you think?

> (Unfortunately, the guy who wrote most of the migrator, Argyrios, just
> went on vacation. I'll CC him but I don't expect him to reply anytime
> soon.)
>
> Jordan