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

[Richard Smith]

On Mon, Jun 11, 2012 at 8:58 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> Hi all,
>
> The -verify option has the potential for uses outside just the clang
> test-suite, for example, within a test-suite for a 3rd-party library.
> However, it has a number of idiosyncrasies or missing features which the
> attached patch seeks to address.  While none of the alterations are perhaps
> critical to the development of clang itself (that remains to be seen!),
> they
> do make the -verify feature immeasurably more flexible and useful.
>
> In the first instance, the patch enables -verify to distinguish between
> blocks of code which have been #if/#ifdef-ed out and those which have not.
> The current implementation will not do this, so for example, this trivial
> fragment will cause an "expected but not seen" failure:
>
> #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.

Secondly, in the current implementation, all diagnostics must be contained
> on the actual line which generates the diagnostic.  The new implementation
> allows a line number to be optionally specified as part of the diagnostic
> declaration using the @ symbol, for example:
>
> // expected-error@<line> {{some error}}
>
> where <line> can be some absolute line number, for example: 10, or a
> relative line number, for example: -1 or +5, from the current line.  Note
> that where an absolute line number is given this is intentionally the
> actual
> source line in the file in question, not one modified by the #line
> directive.  Likewise, relative line numbers are also not affected by the
> #line directive.
>

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.


> The patch also includes the relevant modifications necessary to the
> existing
> clang test-suite.  These modifications were necessary for two reasons:
> since
> #if-block processing is now included, various tests in the PCH folder were
> now missing their relevant diagnostics.


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).

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

--- 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.

 Also #error/#warning directives
> include all text up to the end-of-line in their message so tests containing
> these have the diagnostics shifted onto a separate line now.
>

That looks like a bug. Comments are supposed to be "replaced by one space
character" in the phase of translation preceding the expansion of
preprocessing directives, so should not appear in the diagnostic.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120611/f9427b24/attachment.html>