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 testsanyway. Even though it doesn't actually cause any problems. I don't thinkthis 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! I have reworked this patch from another angle. It still works towards the same aim (i.e. that of making -verify diagnostics show following a fatal error in the test-case), but does so without exposing a public interface that escalates a suppress / override / really suppress / really override scenario. This patch works by providing one public interface point which is setForceEmit() inside DiagnosticBuilder. Its use would be something like: Diags.Report(diag::err_something).setForceEmit() << Data1 << Data2; This approach means that there isn't a global flag in the diagnostics engine that could be set and then accidentally not reset: it is only valid for the diagnostic under construction. The user of this interface should not be predisposed to consider its use except in specific instances (for example, -verify), and certainly has to consider each use separately as he cannot simply affect a whole set of diagnostics, as was the case with the previous interface. If you look through the patch, I've added an internal method to DiagnosticsEngine, ForceEmitCurrentDiagnostic, that closely mirrors the existing EmitCurrentDiagnostic method, except that it bypasses the checks on suppressions. DiagnosticIDs::ProcessDiag has been split at the end into a separate DiagnosticIDs::EmitDiag method which is then used by ForceEmitCurrentDiagnostic. In the process, I also simplified the code in ProcessDiag where it checks the diagnostic level since a dedicated function exists for this purpose. I am ready for you to say this is no better than the last patch, but I thought I'd submit it to your scrutiny which is (as ever) highly valued! Of course, I can go in at a different level with this and implement it all within VerifyDiagnosticConsumer by calling HandleDiagnostic directly on its own client, but I think this becomes a very messy approach, even though it doesn't open up the possibility of abuse elsewhere... If you still don't think this is a good direction to go in, I will accept leaving it out of my patch set. Cheers Andy
verify-part4.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
