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

[Richard Smith]

On Mon, Jun 11, 2012 at 12:55 PM, Andy Gibbs <andyg1001 at hotmail.co.uk>wrote:

> **
> 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 at +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.)
>

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.

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

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.


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

> > 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?
>
> Because the preprocessor was unable to "see" the comment in this situation.
>

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.

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

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120611/780fef2b/attachment.html>