[cfe-commits] [cfe-dev] Patch for review: enhancements/fixes to -verify (resubmission)

[Richard Smith]

Hi Andy,

On Wed, Jun 13, 2012 at 8:17 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
> Attached is a resubmission of my -verify enhancement patch.
>
> There are two main differences between this version and the previous one at
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120611/058799.html:
>
> 1. Following the patches for bugs in the preprocessor posted at:
>     http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022037.html
>     http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022044.html
>   the needed changes to pre-existing test-cases has been brought to
>   a minimum.  ***NOTE*** this patch won't apply without these first
>   being applied!!!
>
> 2. I have added an extensive test-case to (hopefully) demonstrate that
>   there should be no erroneous swallowing of expected-* checks, as
>   was Richard Smith's concern.

Thanks, the test cases look great.

> Since this is a resubmission, and there has been some discussion on
> what the functionality should be, I will quickly outline what the
> main changes for the new -verify implementation are:
>
> 1. Diagnostic checks can now be filtered out along with the lines
>   that generate the diagnostics, when they are encapsulated inside
>   a #if block that is skipped.  This enables conditional checks.
>
> 2. It is also now possible to place expected-* checks on separate
>   lines to that which creates the diagnostic.  A line number can
>   be specified in the check, and can be either absolute (i.e. to
>   the actual line in the source file) or relative to the current
>   line.
>
> 3. There is additional flexibility in the "number of occurrences"
>   feature.  In addition to being able to specify a fixed number
>   or "one or more", it is now possible to specify "n or more"
>   and "between n and m".  As mentioned in one of the earlier
>   posts, this is not necessarily a feature that may find much
>   use for writing clang test-cases, but is a useful feature for
>   anyone wishing to use -verify to test their own code.

OK, I think you've made a good case for all these features. However,
you are much more likely to get them reviewed if you can provide
separate patches for each feature.

> The following changes were necessary in pre-existing test-cases:
>
> 1. Where PCHs are involved, the expected-* checks needed to be
>   moved into the actual test-case rather than the "header".

OK.

> 2. If a fatal error occurs in a test-case, then in the current
>   implementation there will be no diagnostics from the -verify
>   function either.  This is fixed in the new implementation.

Nice catch.

> 3. It could happen that the current implementation would not gather
>   all the expected-* checks from included header files since it
>   only considered the main file and one other FileID record.  The
>   new implementation considers all FileID records, marking them
>   "unseen" if a diagnostic occurs with them, and "seen" if a
>   expected-* check is found within them.  At the end, if there
>   exists any files that are "unseen" and not "seen", then these
>   are parsed again for expected-* checks.  This should limit the
>   likelihood of missed expected-* checks, and did reveal an error
>   in test/Modules/on-demand-build.m test-case.

I didn't follow this. It seems to me that we should either (a) only
consider expected-* comments we see when preprocessing the main source
file, or (b) consider all expected-* comments we see in any file (and
the latter makes more sense to me). When does the need for re-parsing
arise?

Thanks!
Richard