<div class="gmail_quote">On Mon, Jun 11, 2012 at 8:58 AM, Andy Gibbs <span dir="ltr"><<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi all,<br>
<br>
The -verify option has the potential for uses outside just the clang<br>
test-suite, for example, within a test-suite for a 3rd-party library.<br>
However, it has a number of idiosyncrasies or missing features which the<br>
attached patch seeks to address.  While none of the alterations are perhaps<br>
critical to the development of clang itself (that remains to be seen!), they<br>
do make the -verify feature immeasurably more flexible and useful.<br>
<br>
In the first instance, the patch enables -verify to distinguish between<br>
blocks of code which have been #if/#ifdef-ed out and those which have not.<br>
The current implementation will not do this, so for example, this trivial<br>
fragment will cause an "expected but not seen" failure:<br>
<br>
#if 0<br>
#error some error // expected-error {{some error}}<br>
#endif<br></blockquote><div><br></div><div>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:</div>
<div><br></div><div>#if __has_feature(foo)</div><div>#error should have foo // expected-error {{should have foo}}</div><div>#endif</div><div> </div><div>However, this may be a price worth paying for the extra convenience of being able to put multiple similar tests in the same file.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Secondly, in the current implementation, all diagnostics must be contained<br>
on the actual line which generates the diagnostic.  The new implementation<br>
allows a line number to be optionally specified as part of the diagnostic<br>
declaration using the @ symbol, for example:<br>
<br>
// expected-error@<line> {{some error}}<br>
<br>
where <line> can be some absolute line number, for example: 10, or a<br>
relative line number, for example: -1 or +5, from the current line.  Note<br>
that where an absolute line number is given this is intentionally the actual<br>
source line in the file in question, not one modified by the #line<br>
directive.  Likewise, relative line numbers are also not affected by the<br>
#line directive.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The patch also includes the relevant modifications necessary to the existing<br>
clang test-suite.  These modifications were necessary for two reasons: since<br>
#if-block processing is now included, various tests in the PCH folder were<br>
now missing their relevant diagnostics.</blockquote><div> </div><div>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).</div>
<div><br></div><div><div>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:</div><div><br></div><div><div>+// expected-error@+2 {{pasting formed '/*', an invalid preprocessing token}}</div>
<div> #define COMM / ## *</div><div>-COMM // expected-error {{pasting formed '/*', an invalid preprocessing token}}</div><div>+COMM</div></div><div><br></div><div>Why is this change necessary?</div><div><br></div>
<div><div>--- test/Preprocessor/if_warning.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 158299)</div><div>+++ test/Preprocessor/if_warning.c<span class="Apple-tab-span" style="white-space:pre">  </span>(working copy)</div>
</div><div><div>@@ -3,7 +3,8 @@</div><div> </div><div> extern int x;</div><div> </div><div>-#if foo   // expected-error {{'foo' is not defined, evaluates to 0}}</div><div>+// expected-error@+1 {{'foo' is not defined, evaluates to 0}}</div>
<div>+#if foo</div><div> #endif</div><div> </div><div> #ifdef foo</div><div>@@ -22,8 +23,9 @@</div><div> </div><div> // rdar://9475098</div><div> #if 0</div><div>-#else 1   // expected-warning {{extra tokens}}</div><div>+#else 1</div>
<div> #endif</div><div>+// expected-warning@-2 {{extra tokens}}</div><div><br></div></div><div>I would have expected both of these to work as-is; neither the #if nor the #else line here is skipped.</div><div><br></div></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  Also #error/#warning directives<br>
include all text up to the end-of-line in their message so tests containing<br>
these have the diagnostics shifted onto a separate line now.<br></blockquote><div><br></div><div>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.</div>
</div>