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

[Andy Gibbs]

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.

Cheers
Andy