[cfe-dev] [cfe-commits] Patch for review: enhancements/fixes to -verify

[Andy Gibbs]

Hi,

On Monday, June 11, 2012 8:31 PM, Richard Smith wrote:

>> [...snip...]
>> #if 0
>> #error some error // expected-error {{some error}}
>> #endif
>
> I think this is neat, but I have a concern that this makes
> it too easy to write tests which unexpectedly check nothing.
> For instance, a test like this becomes a no-op with your
> patch:
>
> #if __has_feature(foo)
> #error should have foo // expected-error {{should have foo}}
> #endif
> 
> However, this may be a price worth paying for the extra
> convenience of being able to put multiple similar tests in
> the same file.

The way around would be...

// expected-error at +2 {{should have foo}}
#if !__has_feature(foo)
#error should have foo
#endif

Of course, it does require that the test-case writer will think to do this, in which case the unexpected no-op might not be written in the first place.  As mentioned in my response to Jordan Rose, perhaps this is where some test-case-writing guidelines on the clang website might come in handy.  (I'm not particularly volunteering here; I expect there will be clang veterans much better suited to this.)

> Another problem with the existing -verify logic is the way
> it deals with diagnostics which appear outside the main
> source file. You must currently annotate the line (in the
> main source file) corresponding to the line in the other
> file on which the diagnostic appears. It would be useful to
> extend this extension to handle expected-error@<file>:<line>,
> where <file> must match (a suffix of?) the relevant file
> name.

I guess this should be possible... assuming an iterator of FileID objects can accessed through SourceManager, or similar, then the correct FileID can be linked to the diagnostic location.  Then the only further change would be to also check FileIDs when running through the diagnostics list during post-processing.  This latter change is probably worth doing anyway (assuming it doesn't break any existing test cases).  I don't have the code in front of me right now -- I'll have a look tomorrow, and see if I have time to do this.
 
> The PCH changes are unfortunate. Is there any way we can get
> this to work properly for those cases? The relevant lines *are*
> included in the TU (through an included file or PCH).

But my understanding is that the generated PCH doesn't include the comments, so they can't be extracted from there.  Someone will be able to correct me here, or may have some other ideas how to get around it.

> Were all the non-PCH test changes actually necessary? I would
> find it very worrying if we are somehow losing expected-foo
> checks in some cases.

I don't believe we are losing any expected-foo checks.  They are all still there, but may have been moved to a different location in the file.  The main reason is, if we have test-case of the style...

#ifndef PASS1
#define PASS1
 /// this part ends up in the PCH
#else
 /// this part tests the PCH
#endif

by necessity the expected-foo checks must be moved out of the first block into the second, unless somehow they can be embedded in the PCH itself.  The extension is, of course, the situation where the PCH is *not* generated out of the test-case itself, but is pre-generated in some other fashion.  In which case, it is clear that the expected-foo checks would need to be part of the test-case (failing that they aren't embedded in the PCH, which at present they aren't, to my understanding).

> For instance:
>
> +// expected-error at +2 {{pasting formed '/*', an invalid preprocessing token}}
> #define COMM / ## *
> -COMM // expected-error {{pasting formed '/*', an invalid preprocessing token}}
> +COMM
>
> Why is this change necessary?

Because the preprocessor was unable to "see" the comment in this situation.  This *may* be a bug in the preprocessor, but that is outside my expertise (see further explanation below).

> --- test/Preprocessor/if_warning.c (revision 158299)
> +++ test/Preprocessor/if_warning.c (working copy)
> @@ -3,7 +3,8 @@
> 
>  extern int x;
> 
> -#if foo   // expected-error {{'foo' is not defined, evaluates to 0}}
> +// expected-error at +1 {{'foo' is not defined, evaluates to 0}}
> +#if foo
>  #endif
> 
>  #ifdef foo
>  @@ -22,8 +23,9 @@
> 
>  // rdar://9475098
>  #if 0
> -#else 1   // expected-warning {{extra tokens}}
> +#else 1
>  #endif
> +// expected-warning at -2 {{extra tokens}}
>
> I would have expected both of these to work as-is; neither
> the #if nor the #else line here is skipped.

Again, this came down to the preprocessor not "seeing" the comments.  I stepped through with a debugger while writing the patch, but I wasn't able to observe an exact reason as to why the preprocessor/lexer didn't call CommentHandler::HandleComment.

The main change that I have made in my patch is to hook into the preprocessor using the CommentHandler mechanism rather than re-parsing the source file during post-processing of the diagnostics list, which was the previous behaviour (which didn't use the full preprocessor, just the lexer).  Since this is the case, it now relies on the preprocessor to correctly expose the comments in the source file(s), which therefore may now expose bugs there (possibly -- again, I don't want to point fingers of blame, especially since the workings of the preprocessor are not something I have particular experience of at present).

The changes made to the test-suite were necessary to ensure no regressions due to my patch.  I checked each change thoroughly to be satisfied that it didn't change the nature of the test and that no diagnostic checks were lost, so I hope I was successful to this end.

Alternatively, we could mark these tests as XFAIL and then, as a separate thing, look at fixing the preprocessor, if that is what needs doing.

What's the best approach, from your point of view?

Cheers
Andy

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120611/4d04ad1d/attachment.html>