[cfe-dev] Fix for preprocessor CommentHandler bug

[Richard Smith]

On Tue, Jun 12, 2012 at 10:07 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
> On Tuesday, June 12, 2012 6:31 PM, David Blaikie wrote:
>> <clippy>It looks like you're trying to fix a bug - would you like to
>> include a test case?</clippy>
>>
>> & is this a fix for: http://llvm.org/bugs/show_bug.cgi?id=13065 by
>> chance? I'm curious what caused the bug as I'd assumed it was due to
>> my commit ( https://llvm.org/svn/llvm-project/cfe/trunk@158093 ). But
>> judging by the fix, that might not be the case.
>
> No, the bug was uncovered while making an enhancement patch for the
> "-verify" feature, where I changed from the use of the separate parse step
> to using the CommentHandler interface.  Unfortunately, the CommentHandler
> interface was not invoked in some cases where comments followed #else or
> #endif directives, and this meant I needed to alter some pre-existing
> test-cases to enable them to work with my patch.  This fix was directed at
> removing this necessity.
>
> In reply, then, to the missing test-cases, I have test-cases as part of my
> patch for the new "-verify" feature, since this actually uses the
> CommentHandler interface and can directly test it.  I accept that this is a
> weak reply, but my hope is that with the approval of this and my other
> recent preprocessor patch, then my "-verify" patch can also meet with
> approval -- in which case the test-cases will appear as part of that.
>
> Alternatively, is there a better way to test the CommentHandler interface?
> If so, I can supply some specific test-cases for this patch.

Apologies for the delay!

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.

2) Add a tool to tools/ which runs -cc1 over a file and dumps out the
comments found. Then we can use FileCheck to check the results.
c-index-test almost gives us this ability already, by allowing parsing
in comment retention mode, but that doesn't find the same set of
comments as the CommentHandler does (and in particular, it doesn't
find comments in preprocessing directives, which makes it
inappropriate for your purposes).

(1) seems like the simpler option.


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

It also looks like you've changed the behavior of this code:

#if 0
#if 1
#endif junk
#endif

... from producing a warning to being silently accepted. This seems
like an independent bugfix, which should have an accompanying test,
and should probably be committed separately, along with a test that
this:

#if 0
#endif junk

... still warns.