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

[Jordan Rose]

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.