[cfe-dev] Fix for preprocessor CommentHandler bug

[Andy Gibbs]

On Monday, June 18, 2012 2:20 AM, Richard Smith wrote:
> Apologies for the delay!

No problem!  Its only because I'm hoping to get my other patch accepted ;o) 
(By the way, I am also splitting this up, as requested...)

> There are two ways in which you might consider testing
> CommentHandler:
>
> 1) Add a gunit test for it (you should be able to mirror
> the basic structure of unittests/Tooling/RecursiveASTVisitorTest.cpp).
> Your FrontendAction can add a CommentHandler in an
> override of BeginInvocation.

This is the route I've taken.  I should have a new patch together tomorrow 
(I've run out of time for today, unfortunately).

> For the patch itself, would it make sense to move the
> LexingRawMode alteration into CheckEndOfDirective?

Yes, this might make more sense.  I'll have a look into it tomorrow.

> It also looks like you've changed the behavior of this
> code:
>
> #if 0
> #if 1
> #endif junk
> #endif

Well caught!  But it is also the case that other #directives inside an #if 0 
will also not provide these warnings.  Take the following snippet:

#if 0 junk
#if 1 junk
#else junk
#endif junk
#endif junk

Even without my patch, only the first #if and both #endifs produce 
diagnostics.  The way the preprocessor seems to be designed, in most cases 
it simply discards the whole line (comments included) following the 
preprocessor directive when inside a skipped block.  With the change I made, 
one might argue, the diagnostics from the preprocessor are moving towards 
more consistenty: i.e. inside a skipped #if block, there are (or should be) 
no diagnostics generated.  This matches nicely with the fact that we ignore 
comment handling inside skipped #if blocks (thinking ahead to the -verify 
enhancement...)

But on the other hand, of course, I can easily revert the regression.

What do you think?

Thanks
Andy