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

[Andy Gibbs]

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!

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

 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify-part4.diff
Type: application/octet-stream
Size: 9092 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120704/67a97003/attachment.obj>