On Mon, Jul 9, 2012 at 12:01 PM, Jordan Rose <[email protected]> wrote:
> On Jul 4, 2012, at 8:41 , Andy Gibbs <[email protected]> wrote: > > > On Wednesday, July 04, 2012 2:46 AM, Jordan Rose wrote: > >> On Jul 3, 2012, at 4:08 PM, Andy Gibbs wrote: > >>> On Monday, July 02, 2012 8:59 PM, Jordan Rose wrote: > >>> 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. > > > > You mean like this: > > > > Index: tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp > > ================================================================== > > --- tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp > > +++ tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp > > @@ -27,10 +27,11 @@ > > VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine > &_Diags) > > : Diags(_Diags), PrimaryClient(Diags.getClient()), > > OwnsPrimaryClient(Diags.ownsClient()), > > Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(0) > > { > > + assert(PrimaryClient && "PrimaryClient not valid!"); > > Diags.takeClient(); > > } > > > > VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() { > > CheckDiagnostics(); > > @@ -66,10 +67,14 @@ > > FirstErrorFID = SM.getFileID(Info.getLocation()); > > } > > // Send the diagnostic to the buffer, we will check it once we reach > the end > > // of the source file (or are destructed). > > Buffer->HandleDiagnostic(DiagLevel, Info); > > + > > + // Fatal errors should be immediately forwarded to the primary client. > > + if (DiagLevel == DiagnosticsEngine::Fatal) > > + PrimaryClient->HandleDiagnostic(DiagLevel, Info); > > } > > > > > //===----------------------------------------------------------------------===// > > // Checking diagnostics implementation. > > > //===----------------------------------------------------------------------===// > > > > > >> 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). > > > > Yes, you're right, that would have been better. I don't know what I was > > thinking there because I had even considered it, but had oddly thought > that > > it would conflict with Sema::EmitCurrentDiagnostic(unsigned DiagID)! > > > > So, in conclusion, we have the choice now: use the above patch and have > fatal > > errors shown but all other diagnostics suppressed, or I can fix the > patch from > > last time with your comment above and we can use -verify to even check > fatal > > errors. > > > > I am personally happy with either route. I can see advantages both ways. > > I prefer this approach, but I'm hoping someone else can comment here. It's > something that really affects the behavior of -verify. -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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
