[cfe-commits] [Patch 5 of 7] -verify fixes and enhancement

Jordan Rose jordan_rose at apple.com
Tue Jul 10 14:41:38 PDT 2012

On Jul 10, 2012, at 10:25 , Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Monday, July 02, 2012 11:15 PM, Jordan Rose wrote:
>> On Jun 30, 2012, at 14:12 , Andy Gibbs <andyg1001 at hotmail.co.uk> 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?

More information about the cfe-commits mailing list