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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
