<div class="gmail_quote">On Mon, Jun 11, 2012 at 12:55 PM, 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">
<u></u>
<div style="PADDING-LEFT:10px;PADDING-RIGHT:10px;PADDING-TOP:15px" name="Compose message area"><font face="Consolas">
<div>Hi,</div>
<div> </div>
<div>On Monday, June 11, 2012 8:31 PM, Richard Smith wrote:</div>
<div><br>>> [...snip...]<div class="im"><br>>> #if 0<br>>> #error some error
// expected-error {{some error}}<br>>> #endif<br>><br>> I think this
is neat, but I have a concern that this makes<br>> it too easy to write tests
which unexpectedly check nothing.<br>> For instance, a test like this becomes
a no-op with your<br>> patch:<br>><br>> #if __has_feature(foo)<br>>
#error should have foo // expected-error {{should have foo}}<br>>
#endif<br>> <br>> However, this may be a price worth paying for the
extra<br>> convenience of being able to put multiple similar tests in<br>>
the same file.</div></div>
<div> </div>
<div>The way around would be...</div>
<div> </div>
<div>// <a title="mailto:expected-error@+2
STRG + Klicken, um Verknüpfung zu folgen" href="mailto:expected-error@+2" target="_blank">expected-error@+2</a> {{should have foo}}<br>#if
!__has_feature(foo)<br>#error should have foo<br>#endif</div>
<div> </div>
<div>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.)</div></font></div></blockquote><div><br></div><div>Right, my point was not that it's hard to test, but that it's dangerous: it adds to the list of things we need to be careful about. I'm also concerned that we may have existing tests which do something like that, which this change would neuter. If not, then I'm happy to accept that this doesn't happen in practice.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="PADDING-LEFT:10px;PADDING-RIGHT:10px;PADDING-TOP:15px" name="Compose message area"><font face="Consolas"><div class="im">
<div>> The PCH changes are unfortunate. Is there any way we
can get</div></div><div><div class="im">> this to work properly for those cases? The relevant lines
*are*<br>> included in the TU (through an included file or PCH).</div></div>
<div> </div>
<div>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.</div></font></div></blockquote><div><br></div><div>Yes, I suspect this may not be possible. One alternative would be to introduce another -verify mode which looks at comments within #if 0'd out blocks.</div>
<div><span style="font-family:Consolas"> </span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="PADDING-LEFT:10px;PADDING-RIGHT:10px;PADDING-TOP:15px" name="Compose message area">
<font face="Consolas"><div class="im">
<div>> Were all the non-PCH test changes actually necessary? I would<br>>
find it very worrying if we are somehow losing expected-foo<br>> checks in
some cases.</div>
<div> </div>
</div><div>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.</div></font></div></blockquote><div>[...]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="PADDING-LEFT:10px;PADDING-RIGHT:10px;PADDING-TOP:15px" name="Compose message area"><font face="Consolas"><div class="im"><div>> For instance:<br>><br>> +// <a href="mailto:expected-error@+2" target="_blank">expected-error@+2</a> {{pasting formed '/*', an
invalid preprocessing token}}<br>> #define COMM / ## *<br>> -COMM //
expected-error {{pasting formed '/*', an invalid preprocessing token}}<br>>
+COMM<br>><br>> Why is this change necessary?</div>
<div> </div>
</div><div>Because the preprocessor was unable to "see" the comment in this
situation.</div></font></div></blockquote><div><br></div><div>This is exactly what I meant by "losing" expected-foo checks -- any new markers added in such places will have no effect. For what it's worth, I think this is at least partially a feature of these changes: they're allowing us to more directly test our CommentHandler logic, which appears to be broken in various ways. Nonetheless, we should fix the preprocessor rather than working around it in the tests.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="PADDING-LEFT:10px;PADDING-RIGHT:10px;PADDING-TOP:15px" name="Compose message area"><font face="Consolas"><div>
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.</div>
<div> </div>
<div>What's the best approach, from your point of view?</div></font></div></blockquote></div><br><div>The @line part seems to be uncontroversial -- if you can provide a patch for just that part, we can work to get that checked in first. I'm not particularly comfortable basing -verify on CommentHandler while it's known to have dropped-comment bugs, so I'd prefer to get it fixed before taking the CommentHandler change. Assuming that we're happy to change all the PCH tests per your patch, how many tests would we have to XFAIL?</div>