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

[Richard Smith]

On Mon, Jun 25, 2012 at 9:34 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> 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?


> Part 2: Implement alternative line number syntax
>

+    // next optional token: @

Comments should start with a capital letter (and usually end with a full
stop).

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

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


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

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.

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

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.


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


> Part 7: Test cases


 These should be split up to go with the related code changes above.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120627/86b82acf/attachment.html>