[cfe-dev] Patch for review: enhancements/fixes to -verify (resubmission)
Richard Smith
richard at metafoo.co.uk
Sun Jun 17 19:53:48 PDT 2012
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
More information about the cfe-dev
mailing list