[cfe-commits] [Patch 1 of 7] -verify fixes and enhancement

[Andy Gibbs]

On Thursday, June 28, 2012 2:12 AM, Richard Smith wrote:
> On Mon, Jun 25, 2012 at 9:34 AM, Andy Gibbs wrote:
> 
>> Hi,
>> 
>> Attached is the first of seven patches for the -verify feature, split up
>> as requested!
>>
>> The division is as follows:
>> 
>> Part 1: preparation; make ExpectedData a member variable to
>>         VerifyDiagnosticConsumer instance and move Directive
>>         and ExpectedData classes into header.
>
> Is there a reason to make these member classes public? Seems fine otherwise.
> This is needed by part 5, right?

Correct.

>> Part 2: Implement alternative line number syntax
>
> +    // next optional token: @
>
> Comments should start with a capital letter (and usually end with a full
> stop).

No problem; will do.

> +    SourceLocation RealPos = Pos;
>
> This name could be clearer. Maybe ExpectedLoc, to indicate this is the
> location where we expect the diagnostic? Where does the 'verify failed'
> diagnostic point in the case when the expectation isn't met? Perhaps we 
> should maintain two SourceLocations in a Directive, to indicate where the
> directive was written and where the diagnostic is expected, so we can point
> at the directive itself in that case?

Oh, I thought RealPos was fairly clear, but accept that your name is better!
I agree also with the idea of keeping both locations -- I will look into it.

> +      if (PH.Next("-")) { // previous to current line
>
> Please put the comment on a line of its own.
>
> +      } else if (PH.Next("+")) { // subsequent to current line
>
> I'd prefer that the "-" and "+" cases be merged together, since their
> implementations are nearly identical.

Ok, will do.
 
>> Part 3: Implement ranges for diagnostic appearances
>
> +      // a positive integer can be followed by a '+'
>
> As above, please start this comment with a capital letter.
> 
> +        if (PH.Next(Max)) PH.Advance();
> +        if (Max < Min) Max = Min;
>
> Maybe we should issue a diagnostic in the 'else' cases here?

Ok.

>> Part 4: Fix the swallowing of verify diagnostics following
>>         diagnostics causing suppression - e.g. fatal error
>
> @@ -634,9 +634,10 @@
> 
>    // If the client doesn't care about this message, don't issue it.  If this is
>    // a note and the last real diagnostic was ignored, ignore it too.
> -  if (DiagLevel == DiagnosticIDs::Ignored ||
> -      (DiagLevel == DiagnosticIDs::Note &&
> -       Diag.LastDiagLevel == DiagnosticIDs::Ignored))
> +  if (!Diag.ForceEmitDiagnostics &&
> +        (DiagLevel == DiagnosticIDs::Ignored ||
> +          (DiagLevel == DiagnosticIDs::Note &&
> +           Diag.LastDiagLevel == DiagnosticIDs::Ignored)))
>      return false;
>
> I don't think we want this change; I think we should only be overriding
> diagnostic limits, not -Wno-foo. In particular, we have code which checks
> whether a diagnostic is enabled before computing whether to emit the
> diagnostic, and this change makes that not be a pure optimization any more.
> In any case, we still want to suppress notes if the preceding diagnostic
> was suppressed.

Hmm, this looks like it might be in by mistake.  My apologies; I have many
patches in by own branch and occasionally they cross over, though I do double-
check to try and avoid this.  This is (will be) part of my patch for caching
constexpr function evaluations, which I am ready to post when all the groundwork
patches are in.  Lets take it out for now, we can discuss it again when I can
demonstrate its purpose more obviously.  Maybe you will suggest an alternative,
in which case this change is not needed.

(As a side note, ForceEmitDiagnostics is expected only to be used as a wrapper
around a single diagnostic, where it is determined that whatever suppressions
might otherwise be in place, this diagnostic must be emitted anyway.)

> The rest of this part looks fine.
>  
> Part 5: Implement conditional diagnostic checking, moving
>         from post-processing of source file to using the
>         CommentHandler interface
>
> The names Seen and Unseen are quite opaque to me. FilesWithDiagnostics and
> FilesWithDirectives would be much clearer.

Ok, will do.  Your names are certainly better.

> I also don't yet understand why we would get into the situation where we need
> the special-case code to scan the FilesWithDiagnostics which we didn't find
> directives in. Is this related to modules / PCH / PPH? Can you point out an
> example test where this comes up?

In short, yes, the problems relate to modules and pre-compiled headers, and
as in the case of ARCMT.cpp, where there is a separate parse process.

To quote from my other mail on this:

"In an ideal world there should be no mismatch at the end of processing 
 between the "seen" and "unseen" lists.  However, this is still not the
 case (my  implementation while arguably better is still not perfect)
 since *some* synthesised include files are still not "seen".  In the
 case of ARCMT.cpp, I have fixed this but other cases still exist.
 Therefore, I have a backup which scans the unseen list and ensures that
 these files are processed at the end even though they were missed first
 time around.  In the end, we can still guarantee that all source files
 have been seen."

I believe there should be ways of tidying up these loose ends (for want of a
better term!), but I am hard-pressed to find the time to do this right now. In
a month's time I should be able to look into it, but I really don't want this
to be a hold point to these patches.  The position I have left it in is safe:
files falling through the first net are caught in the second -- still better
than the current situation where there is only one net, which has a few holes
in it already!

Can we put a FIXME marker in the source at this point?  When fixed, I would
still suggest that under a debug build (#ifndef NDEBUG) we still scan through
the two lists and assert on mismatch.

I haven't got my code in front of me, so I can't give you exact examples of
test case failures, but you can always just insert an assert, and see where
the errors occur...  They will occur in test/ARCMT, test/Modules and test/PCH.

> I also don't like the coupling between DiagnosticConsumer and CommentHandler
> which the getCommentHandler method introduces. This seems to exist only to
> support part 6, so I wonder if there's a better way to approach that.

Yes, this is in support of part 6.  I am open to suggestions for a better
approach, but it does need to be available somehow to a sub-class of
DiagnosticConsumer.  Introducing getCommentHandler seemed the most obvious
way.
 
> Part 6: Update CaptureDiagnosticConsumer in ARCMT.cpp to
>         also capture and forward the comment lines, as
>         well as the diagnostics
>
> We need this to fix ARCMT tests broken by part 5, right? I don't know anything
> about this code, so hopefully someone else will look over these changes.

Actually, this *doesn't* fix any broken tests since these tests are not broken
by my part 5 patch.  This patch is provided to move away from needing the scan
of unseen files in that patch, as is both our wishes.
 
> Part 7: Test cases
>
> These should be split up to go with the related code changes above.

You could link them then to the part 5 patch, since this is the one which
requires the changes to be made.  All the other patches do not introduce any
regressions.

I will update my patches according to your comments, and repost them tonight.

Thanks for all your help.

Andy

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120628/9d4e7ce9/attachment.html>