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

[Jordan Rose]

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?
Jordan