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

[Jordan Rose]

On Jul 3, 2012, at 4:08 PM, Andy Gibbs wrote:

> On Monday, July 02, 2012 8:59 PM, Jordan Rose wrote:
>> On Jul 2, 2012, at 11:45 , Andy Gibbs wrote:
>>> On Monday, July 02, 2012 7:48 PM, Jordan Rose wrote:
>>>> 
>>>> This one I don't like. -verify tests shouldn't also be fatal error tests
>>>> anyway. Even though it doesn't actually cause any problems. I don't think
>>>> this is necessary.
>>>> 
>>>> (Also, it invites future code like ReallySuppressAllDiagnostics, etc.,
>>>> even if none of us would ever do that.)
>>> 
>>> I take your point, but  my counter argument would be that it may be very
>>> possible to have the scenario where a test-case that originally didn't
>>> fatal error, might following an unintentional change the the compiler end
>>> up in a fatal error and which is then missed it because the diagnostics
>>> following the fatal error get swallowed up.  Right now I can't be 100%
>>> sure but I think this actually came up during testing. It was certainly
>>> one of the later iterations of the patch that included this change.
>> 
>> I'd rather just make this a rule: "fatal errors can't be tested with
>> -verify", and have them not be suppressed by VerifyDiagnosticsConsumer. I
>> guess this probably requires further discussion either way.
> 
> 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.

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). I can live with it. (Anyone else have opinions here?)

Jordan