<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META content=text/html;charset=iso-8859-1 http-equiv=Content-Type>
<META name=GENERATOR content="MSHTML 8.00.7601.17785"></HEAD>
<BODY style="PADDING-LEFT: 10px; PADDING-RIGHT: 10px; PADDING-TOP: 15px"
id=MailContainerBody leftMargin=0 topMargin=0 CanvasTabStop="true"
name="Compose message area"><FONT size=2 face=Consolas>
<DIV>Hi,</DIV>
<DIV> </DIV>
<DIV>On Monday, June 11, 2012 8:31 PM, Richard Smith wrote:</DIV>
<DIV><BR>>> [...snip...]<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>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">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>
<DIV> </DIV>
<DIV>> Another problem with the existing -verify logic is the way<BR>> it
deals with diagnostics which appear outside the main<BR>> source file. You
must currently annotate the line (in the<BR>> main source file) corresponding
to the line in the other<BR>> file on which the diagnostic appears. It would
be useful to<BR>> extend this extension to handle <A
title="mailto:expected-error@<file>:<line
STRG + Klicken, um Verknüpfung zu folgen"
href="mailto:expected-error@<file>:<line">expected-error@<file>:<line</A>>,<BR>>
where <file> must match (a suffix of?) the relevant file<BR>>
name.</DIV>
<DIV> </DIV>
<DIV>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.<BR> <BR>> The PCH changes are unfortunate. Is there any way we
can get<BR>> 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>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>
<DIV> </DIV>
<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>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...</DIV>
<DIV> </DIV>
<DIV>#ifndef PASS1<BR>#define PASS1<BR> /// this part ends up in
the PCH<BR>#else<BR> /// this part tests the PCH<BR>#endif</DIV>
<DIV> </DIV>
<DIV>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).</DIV>
<DIV> </DIV>
<DIV>> For instance:<BR>><BR>> +// <A
href="mailto:expected-error@+2">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>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).</DIV>
<DIV> </DIV>
<DIV>> --- test/Preprocessor/if_warning.c (revision 158299)<BR>> +++
test/Preprocessor/if_warning.c (working copy)<BR>> @@ -3,7 +3,8 @@<BR>>
<BR>> extern int x;<BR>> <BR>> -#if foo //
expected-error {{'foo' is not defined, evaluates to 0}}<BR>> +// <A
href="mailto:expected-error@+1">expected-error@+1</A> {{'foo' is not defined,
evaluates to 0}}<BR>> +#if foo<BR>> #endif<BR>> <BR>>
#ifdef foo<BR>> @@ -22,8 +23,9 @@<BR>> <BR>> //
rdar://9475098<BR>> #if 0<BR>> -#else 1 //
expected-warning {{extra tokens}}<BR>> +#else 1<BR>> #endif<BR>>
+// <A href="mailto:expected-warning@-2">expected-warning@-2</A> {{extra
tokens}}<BR>><BR>> I would have expected both of these to work as-is;
neither<BR>> the #if nor the #else line here is skipped.</DIV>
<DIV> </DIV>
<DIV>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.</DIV>
<DIV> </DIV>
<DIV>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).</DIV>
<DIV> </DIV>
<DIV>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.</DIV>
<DIV> </DIV>
<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>
<DIV> </DIV>
<DIV>Cheers</DIV>
<DIV>Andy</DIV>
<DIV> </DIV>
<DIV></FONT> </DIV></BODY></HTML>