r212191 - ARCMigrate: simplify diagnostic handling

[Alp Toker]

On 02/07/2014 20:25, David Blaikie wrote:
> On Wed, Jul 2, 2014 at 10:08 AM, Alp Toker <alp at nuanti.com> wrote:
>> Author: alp
>> Date: Wed Jul  2 12:08:00 2014
>> New Revision: 212191
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=212191&view=rev
>> Log:
>> ARCMigrate: simplify diagnostic handling
>>
>> Recent enhancements in the diagnostics engine mean that
>> TransformActions::report() no longer needs to duplicate this suppression logic.
>>
>> That's great because the old code was flawed and would have attached notes to
>> the wrong primary diagnostic in non-trivial use.
> Any value in adding a test for this? (probably not, now that the
> codepaths are shared and presumably the surviving/existing codepath
> was already well/better tested - but just checking)

Right. I'm comfortable enough deferring to existing test coverage.

>> With these changes it becomes safe to use reportNote() freely in the migration
>> tool.

Of course I'd encourage the authors to try using reportNote() to enhance 
QOI in a couple more places now that it works, which should extend test 
coverage naturally. (I suspect it's so far been avoided due to the 
attachment bug.)

Alp.


>>
>> Modified:
>>      cfe/trunk/include/clang/Basic/Diagnostic.td
>>      cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td
>>      cfe/trunk/lib/ARCMigrate/Internals.h
>>      cfe/trunk/lib/ARCMigrate/TransformActions.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/Diagnostic.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.td?rev=212191&r1=212190&r2=212191&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/Diagnostic.td (original)
>> +++ cfe/trunk/include/clang/Basic/Diagnostic.td Wed Jul  2 12:08:00 2014
>> @@ -89,6 +89,10 @@ class ShowInSystemHeader {
>>     bit ShowInSystemHeader = 1;
>>   }
>>
>> +class SuppressInSystemHeader {
>> +  bit ShowInSystemHeader = 0;
>> +}
>> +
>>   // FIXME: ExtWarn and Extension should also be SFINAEFailure by default.
>>   class Error<string str>     : Diagnostic<str, CLASS_ERROR, SEV_Error>, SFINAEFailure {
>>     bit ShowInSystemHeader = 1;
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td?rev=212191&r1=212190&r2=212191&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td Wed Jul  2 12:08:00 2014
>> @@ -138,7 +138,7 @@ def err_module_file_conflict : Error<"mo
>>
>>   // TransformActions
>>   // TODO: Use a custom category name to distinguish rewriter errors.
>> -def err_mt_message : Error<"[rewriter] %0">;
>> +def err_mt_message : Error<"[rewriter] %0">, SuppressInSystemHeader;
>>   def warn_mt_message : Warning<"[rewriter] %0">;
>>   def note_mt_message : Note<"[rewriter] %0">;
>>
>>
>> Modified: cfe/trunk/lib/ARCMigrate/Internals.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/Internals.h?rev=212191&r1=212190&r2=212191&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/ARCMigrate/Internals.h (original)
>> +++ cfe/trunk/lib/ARCMigrate/Internals.h Wed Jul  2 12:08:00 2014
>> @@ -48,7 +48,6 @@ void writeARCDiagsToPlist(const std::str
>>   class TransformActions {
>>     DiagnosticsEngine &Diags;
>>     CapturedDiagList &CapturedDiags;
>> -  bool ReportedErrors;
>>     void *Impl; // TransformActionsImpl.
>>
>>   public:
>> @@ -104,7 +103,9 @@ public:
>>     void reportNote(StringRef note, SourceLocation loc,
>>                     SourceRange range = SourceRange());
>>
>> -  bool hasReportedErrors() const { return ReportedErrors; }
>> +  bool hasReportedErrors() const {
>> +    return Diags.hasUnrecoverableErrorOccurred();
>> +  }
>>
>>     class RewriteReceiver {
>>     public:
>>
>> Modified: cfe/trunk/lib/ARCMigrate/TransformActions.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/TransformActions.cpp?rev=212191&r1=212190&r2=212191&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/ARCMigrate/TransformActions.cpp (original)
>> +++ cfe/trunk/lib/ARCMigrate/TransformActions.cpp Wed Jul  2 12:08:00 2014
>> @@ -601,7 +601,7 @@ TransformActions::RewriteReceiver::~Rewr
>>   TransformActions::TransformActions(DiagnosticsEngine &diag,
>>                                      CapturedDiagList &capturedDiags,
>>                                      ASTContext &ctx, Preprocessor &PP)
>> -  : Diags(diag), CapturedDiags(capturedDiags), ReportedErrors(false) {
>> +    : Diags(diag), CapturedDiags(capturedDiags) {
>>     Impl = new TransformActionsImpl(capturedDiags, ctx, PP);
>>   }
>>
>> @@ -677,17 +677,6 @@ DiagnosticBuilder TransformActions::repo
>>                                              SourceRange range) {
>>     assert(!static_cast<TransformActionsImpl *>(Impl)->isInTransaction() &&
>>            "Errors should be emitted out of a transaction");
>> -
>> -  SourceManager &SM = static_cast<TransformActionsImpl *>(Impl)
>> -                          ->getASTContext()
>> -                          .getSourceManager();
>> -  DiagnosticsEngine::Level L = Diags.getDiagnosticLevel(diagId, loc);
>> -  // TODO: Move this check to the caller to ensure consistent note attachments.
>> -  if (L == DiagnosticsEngine::Ignored ||
>> -      SM.isInSystemHeader(SM.getExpansionLoc(loc)))
>> -    return DiagnosticBuilder::getEmpty();
>> -  if (L >= DiagnosticsEngine::Error)
>> -    ReportedErrors = true;
>>     return Diags.Report(loc, diagId) << range;
>>   }
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-- 
http://www.nuanti.com
the browser experts