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

[Richard Smith]

On Mon, Jul 9, 2012 at 12:01 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> On Jul 4, 2012, at 8:41 , Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
>
> > On Wednesday, July 04, 2012 2:46 AM, Jordan Rose wrote:
> >> On Jul 3, 2012, at 4:08 PM, Andy Gibbs wrote:
> >>> On Monday, July 02, 2012 8:59 PM, Jordan Rose wrote:
> >>> I think a problem I have seen is that where a fatal error occurs, you
> >>> simply get no diagnostics at all -- except a cryptic line telling you
> how
> >>> many errors occurred.  From my point of view it is much better to
> always
> >>> see the -verify diagnostics since this is the purpose of the feature --
> >>> to catch diagnostics and display them where they do not match those
> >>> expected.  In the current case, you don't even get the diagnostic of
> the
> >>> line *causing* the fatal error!
> >>
> >> Again, what I'd prefer is that fatal errors are NOT suppressed, or
> rather
> >> not caught by VerifyDiagnosticsConsumer, so even if you don't get the
> >> verify-related complaints you still know what's up. Something as simple
> as
> >> a check in HandleDiagnostic for DiagLevel == FatalError, and then a very
> >> simple text output on the spot would do it for me.
> >
> > You mean like this:
> >
> > Index: tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
> > ==================================================================
> > --- tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
> > +++ tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
> > @@ -27,10 +27,11 @@
> > VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine
> &_Diags)
> >   : Diags(_Diags), PrimaryClient(Diags.getClient()),
> >     OwnsPrimaryClient(Diags.ownsClient()),
> >     Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(0)
> > {
> > +  assert(PrimaryClient && "PrimaryClient not valid!");
> >   Diags.takeClient();
> > }
> >
> > VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() {
> >   CheckDiagnostics();
> > @@ -66,10 +67,14 @@
> >     FirstErrorFID = SM.getFileID(Info.getLocation());
> >   }
> >   // Send the diagnostic to the buffer, we will check it once we reach
> the end
> >   // of the source file (or are destructed).
> >   Buffer->HandleDiagnostic(DiagLevel, Info);
> > +
> > +  // Fatal errors should be immediately forwarded to the primary client.
> > +  if (DiagLevel == DiagnosticsEngine::Fatal)
> > +    PrimaryClient->HandleDiagnostic(DiagLevel, Info);
> > }
> >
> >
> //===----------------------------------------------------------------------===//
> > // Checking diagnostics implementation.
> >
> //===----------------------------------------------------------------------===//
> >
> >
> >> However, if we don't go with that, this approach is better than the
> last one
> >> (though I would have personally used a flag argument on
> EmitCurrentDiagnostic
> >> rather than a separate ForceEmitCurrentDiagnostic).
> >
> > Yes, you're right, that would have been better.  I don't know what I was
> > thinking there because I had even considered it, but had oddly thought
> that
> > it would conflict with Sema::EmitCurrentDiagnostic(unsigned DiagID)!
> >
> > So, in conclusion, we have the choice now: use the above patch and have
> fatal
> > errors shown but all other diagnostics suppressed, or I can fix the
> patch from
> > last time with your comment above and we can use -verify to even check
> fatal
> > errors.
> >
> > I am personally happy with either route.  I can see advantages both ways.
>
> I prefer this approach, but I'm hoping someone else can comment here. It's
> something that really affects the behavior of -verify.


-verify already works when checking fatal errors, and is currently used for
that by several tests. What doesn't work currently is printing out the
-verify summary after a fatal error occurs, if there are mismatches between
expectations and diagnostics. So let's just fix that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120709/1a86e0ad/attachment.html>