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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
