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

Jordan Rose jordan_rose at apple.com
Tue Jul 24 11:05:11 PDT 2012


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.

Is it possible to just add a "shouldCaptureDiagnostics" argument to arcmt::checkForManualIssues? I think that's really the cleanest way to solve the problem.

(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


On Jul 17, 2012, at 9:19 AM, Andy Gibbs wrote:

> This is patch 2, which fixes CaptureDiagnosticConsumer inside ARCMT.cpp in what I hope is considered a more acceptible way.
> 
> The issue comes because CaptureDiagnosticConsumer must now additionally capture expected-* directives and hold onto them pending forwarding them onto VerifyDiagnosticConsumer along with the diagnostics that have been generated.  (In anticipation of the question, why not forward comments immediately, it is the same reason why diagnostics cannot be forwarded immediately...)
> 
> In order to do this, in a previous patch, I created a work-around so that the CommentHandler interface of VerifyDiagnosticConsumer could be found.  This was rightly considered a hack by my reviewers, so I have fully implemented dyn_cast into DiagnosticConsumer classes, so that such a hack is not necessary, plus it is now possible to use isa/cast/dyn_cast which may find uses elsewhere!
> 
> The patch is large, but is mostly the implementation of dyn_cast and friends; the patch to ARCMT.cpp is very similar to the original patch posted previously.
> 
> A note to my implementation for dyn_cast:
> 
> DiagnosticConsumer constructor now takes a class identifier, which is defined inside DiagnosticConsumer as an enum.  For subclasses of DiagnosticConsumer which are available outside a single translation unit, I have assigned class identifiers.  For those which are only used internally, I have assigned an "unknown" class identifier.  These classes cannot then be used with dyn_cast et al.  For those subclassing DiagnosticConsumer outside libclang, i.e. in their own projects (since I assume this is possible), this "unknown" class identifier should also be used there since changes to Diagnostic.h would be deemed inappropriate.
> 
> Cheers
> Andy
> 
> <patch2-arcmt.diff>




More information about the cfe-commits mailing list