On Jul 10, 2012, at 10:25 , Andy Gibbs <[email protected]> wrote:
> On Monday, July 02, 2012 11:15 PM, Jordan Rose wrote: >> On Jun 30, 2012, at 14:12 , Andy Gibbs <[email protected]> wrote: >> >>> Part 5: Renamed Seen and Unseen to FilesWithDirectives and >>> FilesWithDiagnostics as requested; moved changes specific to part 6 >>> into part 6 patch; added FIXME on post-process loop. >>> >>> This patch must be combined with part 7 which includes the test-cases. >>> <verify-part5.diff> >> >> // FIXME: Const hack, we screw up the preprocessor but in practice its ok >> // because it doesn't get reused. It would be better if we could make a copy >> // though. >> CurrentPreprocessor = const_cast<Preprocessor*>(PP); >> + if (CurrentPreprocessor) CurrentPreprocessor->addCommentHandler(this); >> >> PrimaryClient->BeginSourceFile(LangOpts, PP); >> } >> >> void VerifyDiagnosticConsumer::EndSourceFile() { >> + if (CurrentPreprocessor) CurrentPreprocessor->removeCommentHandler(this); >> >> The "const hack" isn't actually being used in current code, but >> would be necessary to add and remove your comment handler. However, >> we'll be able to take this out completely after the tests are updated, >> right? > > Not exactly, in the sense that add/removeCommentHandler still requires non- > const access. But I don't believe we're "screwing up" the preprocessor, so > I've taken out the "FIXME". > > Apart from the add/removeCommentHandler interface, no non-const access is > made to CurrentPreprocessor, and once the post-processing loop is removed > then CurrentPreprocessor will only be used for this purpose. > > As a side-note, if EndSourceFile could be altered to pass the Preprocessor > as an argument like BeginSourceFile then CurrentPreprocessor becomes utterly > obsolete (and along with it, the unsightly const_cast). It's arguable whether add/removeCommentHandler is really changing the state of the preprocessor, but I guess that's a separate discussion. But that's what's really keeping the const_casts around, not the CurrentPreprocessor field. (The real solution might be to have CompilerInstance::createPreprocessor and arcmt-test's checkForMigration functions manually chain in VerifyDiagnosticConsumer as a CommentHandler, and then not worry about this per-source-file thing at all.) >> + std::string C(SM.getCharacterData(Comment.getBegin()), >> + SM.getCharacterData(Comment.getEnd())); >> >> StringRef :-) But I'm wondering now if this can share any code with Dmitri's >> doc-comment-parsing work, at least for \<EOL> folding. > > Changed to StringRef as suggested. I've had a look over the recent changes > regarding comment parsing and I will consider moving over to this code once > it has settled down a little (I note that revisions are still being made to > it…). Fair enough. It might still be worth fast-pathing the case where there are no backslashes (to avoid an extra copy to stack). I'm only realizing now that ParseDirective should probably take a StringRef too. Can you clean that up while you're in there? Sorry to come up with new things to deal with, but that's the point of patch review, right? Jordan _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
