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@+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@+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@+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@-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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
