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