ARCMigrate: Introduce proper diagnostics for TransformActions

This starts to switch ARCMT to use proper diagnostic messages. The old use was
based on incorrect example code from the documentation.

The logic of the previous report() functions has been retained to support any
external consumers that might be intercepting diagnostic messages through the
old interface.

Note that the change in test/Misc/warning-flags.c isn't a new warning without a
flag, rather one that was previously invisible to the test. Adding a flag might
be a good idea though.

llvm-svn: 200124

7 files changed, 50 insertions, 69 deletions

diff --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 152b37c4cf4..09d43f382bb 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -135,4 +135,14 @@ def err_unable_to_make_temp : Error<
 // Modules
 def err_module_file_conflict : Error<"module '%0' found in both '%1' and '%2'">;
 
+// TransformActions
+// TODO: Use a custom category name to distinguish rewriter errors.
+def err_mt_message : Error<"[rewriter] %0">;
+def warn_mt_message : Warning<"[rewriter] %0">;
+def note_mt_message : Note<"[rewriter] %0">;
+
+// ARCMigrate
+def err_arcmt_nsalloc_realloc : Error<"[rewriter] call returns pointer to GC managed memory; it will become unmanaged in ARC">;
+def err_arcmt_nsinvocation_ownership : Error<"NSInvocation's %0 is not safe to be used with an object with ownership other than __unsafe_unretained">;
+
 }
diff --git a/clang/lib/ARCMigrate/ARCMT.cpp b/clang/lib/ARCMigrate/ARCMT.cpp
index b099f08ce93..7894c58ea45 100644
--- a/clang/lib/ARCMigrate/ARCMT.cpp
+++ b/clang/lib/ARCMigrate/ARCMT.cpp
@@ -310,8 +310,11 @@ bool arcmt::checkForManualIssues(CompilerInvocation &origCI,
   TransformActions testAct(*Diags, capturedDiags, Ctx, Unit->getPreprocessor());
   MigrationPass pass(Ctx, OrigGCMode, Unit->getSema(), testAct, capturedDiags,
                      ARCMTMacroLocs);
-  pass.setNSAllocReallocError(NoNSAllocReallocError);
   pass.setNoFinalizeRemoval(NoFinalizeRemoval);
+  Diags->setDiagnosticMapping(diag::err_arcmt_nsalloc_realloc,
+                              NoNSAllocReallocError ? diag::MAP_WARNING
+                                                    : diag::MAP_ERROR,
+                              SourceLocation());
 
   for (unsigned i=0, e = transforms.size(); i != e; ++i)
     transforms[i](pass);
diff --git a/clang/lib/ARCMigrate/Internals.h b/clang/lib/ARCMigrate/Internals.h
index 3690c83d845..d87d1eca1de 100644
--- a/clang/lib/ARCMigrate/Internals.h
+++ b/clang/lib/ARCMigrate/Internals.h
@@ -95,6 +95,8 @@ public:
     return CapturedDiags.hasDiagnostic(IDs, range);
   }
 
+  DiagnosticBuilder report(SourceLocation loc, unsigned diagId,
+                           SourceRange range = SourceRange());
   void reportError(StringRef error, SourceLocation loc,
                    SourceRange range = SourceRange());
   void reportWarning(StringRef warning, SourceLocation loc,
@@ -161,8 +163,6 @@ public:
   const CapturedDiagList &getDiags() const { return CapturedDiags; }
 
   bool isGCMigration() const { return OrigGCMode != LangOptions::NonGC; }
-  bool noNSAllocReallocError() const { return MigOptions.NoNSAllocReallocError; }
-  void setNSAllocReallocError(bool val) { MigOptions.NoNSAllocReallocError = val; }
   bool noFinalizeRemoval() const { return MigOptions.NoFinalizeRemoval; }
   void setNoFinalizeRemoval(bool val) {MigOptions.NoFinalizeRemoval = val; }
 
diff --git a/clang/lib/ARCMigrate/TransAPIUses.cpp b/clang/lib/ARCMigrate/TransAPIUses.cpp
index a0994a6b459..544cb0addfc 100644
--- a/clang/lib/ARCMigrate/TransAPIUses.cpp
+++ b/clang/lib/ARCMigrate/TransAPIUses.cpp
@@ -66,8 +66,7 @@ public:
         selName = "getArgument";
       else if (E->getSelector() == setArgumentSel)
         selName = "setArgument";
-
-      if (selName.empty())
+      else
         return true;
 
       Expr *parm = E->getArg(0)->IgnoreParenCasts();
@@ -75,13 +74,12 @@ public:
       if (pointee.isNull())
         return true;
 
-      if (pointee.getObjCLifetime() > Qualifiers::OCL_ExplicitNone) {
-        std::string err = "NSInvocation's ";
-        err += selName;
-        err += " is not safe to be used with an object with ownership other "
-            "than __unsafe_unretained";
-        Pass.TA.reportError(err, parm->getLocStart(), parm->getSourceRange());
-      }
+      if (pointee.getObjCLifetime() > Qualifiers::OCL_ExplicitNone)
+        Pass.TA.report(parm->getLocStart(),
+                       diag::err_arcmt_nsinvocation_ownership,
+                       parm->getSourceRange())
+            << selName;
+
       return true;
     }
 
diff --git a/clang/lib/ARCMigrate/TransGCCalls.cpp b/clang/lib/ARCMigrate/TransGCCalls.cpp
index 249f20f01b2..401e7880844 100644
--- a/clang/lib/ARCMigrate/TransGCCalls.cpp
+++ b/clang/lib/ARCMigrate/TransGCCalls.cpp
@@ -38,14 +38,8 @@ public:
     TransformActions &TA = MigrateCtx.Pass.TA;
 
     if (MigrateCtx.isGCOwnedNonObjC(E->getType())) {
-      if (MigrateCtx.Pass.noNSAllocReallocError())
-        TA.reportWarning("call returns pointer to GC managed memory; "
-                       "it will become unmanaged in ARC",
-                       E->getLocStart(), E->getSourceRange());
-      else 
-        TA.reportError("call returns pointer to GC managed memory; "
-                       "it will become unmanaged in ARC",
-                       E->getLocStart(), E->getSourceRange());
+      TA.report(E->getLocStart(), diag::err_arcmt_nsalloc_realloc,
+                E->getSourceRange());
       return true;
     }
 
diff --git a/clang/lib/ARCMigrate/TransformActions.cpp b/clang/lib/ARCMigrate/TransformActions.cpp
index 2fd0619df9f..e6268a1792b 100644
--- a/clang/lib/ARCMigrate/TransformActions.cpp
+++ b/clang/lib/ARCMigrate/TransformActions.cpp
@@ -673,60 +673,35 @@ void TransformActions::applyRewrites(RewriteReceiver &receiver) {
   static_cast<TransformActionsImpl*>(Impl)->applyRewrites(receiver);
 }
 
-void TransformActions::reportError(StringRef error, SourceLocation loc,
-                                   SourceRange range) {
-  assert(!static_cast<TransformActionsImpl*>(Impl)->isInTransaction() &&
+DiagnosticBuilder TransformActions::report(SourceLocation loc, unsigned diagId,
+                                           SourceRange range) {
+  assert(!static_cast<TransformActionsImpl *>(Impl)->isInTransaction() &&
          "Errors should be emitted out of a transaction");
 
-  SourceManager &SM = static_cast<TransformActionsImpl*>(Impl)->
-                                             getASTContext().getSourceManager();
-  if (SM.isInSystemHeader(SM.getExpansionLoc(loc)))
-    return;
-
-  // FIXME: Use a custom category name to distinguish rewriter errors.
-  std::string rewriteErr = "[rewriter] ";
-  rewriteErr += error;
-  unsigned diagID
-     = Diags.getDiagnosticIDs()->getCustomDiagID(DiagnosticIDs::Error,
-                                                 rewriteErr);
-  Diags.Report(loc, diagID) << range;
-  ReportedErrors = true;
+  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;
 }
 
-void TransformActions::reportWarning(StringRef warning, SourceLocation loc,
+void TransformActions::reportError(StringRef message, SourceLocation loc,
                                    SourceRange range) {
-  assert(!static_cast<TransformActionsImpl*>(Impl)->isInTransaction() &&
-         "Warning should be emitted out of a transaction");
-  
-  SourceManager &SM = static_cast<TransformActionsImpl*>(Impl)->
-    getASTContext().getSourceManager();
-  if (SM.isInSystemHeader(SM.getExpansionLoc(loc)))
-    return;
-  
-  // FIXME: Use a custom category name to distinguish rewriter errors.
-  std::string rewriterWarn = "[rewriter] ";
-  rewriterWarn += warning;
-  unsigned diagID
-  = Diags.getDiagnosticIDs()->getCustomDiagID(DiagnosticIDs::Warning,
-                                              rewriterWarn);
-  Diags.Report(loc, diagID) << range;
+  report(loc, diag::err_mt_message, range) << message;
 }
 
-void TransformActions::reportNote(StringRef note, SourceLocation loc,
-                                  SourceRange range) {
-  assert(!static_cast<TransformActionsImpl*>(Impl)->isInTransaction() &&
-         "Errors should be emitted out of a transaction");
-
-  SourceManager &SM = static_cast<TransformActionsImpl*>(Impl)->
-                                             getASTContext().getSourceManager();
-  if (SM.isInSystemHeader(SM.getExpansionLoc(loc)))
-    return;
+void TransformActions::reportWarning(StringRef message, SourceLocation loc,
+                                     SourceRange range) {
+  report(loc, diag::warn_mt_message, range) << message;
+}
 
-  // FIXME: Use a custom category name to distinguish rewriter errors.
-  std::string rewriteNote = "[rewriter] ";
-  rewriteNote += note;
-  unsigned diagID
-     = Diags.getDiagnosticIDs()->getCustomDiagID(DiagnosticIDs::Note,
-                                                 rewriteNote);
-  Diags.Report(loc, diagID) << range;
+void TransformActions::reportNote(StringRef message, SourceLocation loc,
+                                  SourceRange range) {
+  report(loc, diag::note_mt_message, range) << message;
 }
diff --git a/clang/test/Misc/warning-flags.c b/clang/test/Misc/warning-flags.c
index 53c9a0397e0..cd7f0a2237b 100644
--- a/clang/test/Misc/warning-flags.c
+++ b/clang/test/Misc/warning-flags.c
@@ -18,7 +18,7 @@ This test serves two purposes:
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (132):
+CHECK: Warnings without flags (133):
 CHECK-NEXT:   ext_delete_void_ptr_operand
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
@@ -95,6 +95,7 @@ CHECK-NEXT:   warn_missing_case_for_condition
 CHECK-NEXT:   warn_missing_dependent_template_keyword
 CHECK-NEXT:   warn_missing_exception_specification
 CHECK-NEXT:   warn_missing_whitespace_after_macro_name
+CHECK-NEXT:   warn_mt_message
 CHECK-NEXT:   warn_multiple_method_decl
 CHECK-NEXT:   warn_no_constructor_for_refconst
 CHECK-NEXT:   warn_not_compound_assign