Allow overriding flag names in diagnostics.

[dnovillo]

Summary

This allows callers of Diags.Report() to override the name of the flag
associated with the diagnostic. This is useful in cases like the -Rpass
flag, where we want the diagnostic to show the name of the pass that
matched the pattern. Instead of showing "... [-Rpass]", this allows
us to show "... [-Rpass=passname]".

[dnovillo]

Instead of an override, provide a flag value.

As we discussed offline, it seems cleaner to offer callers to provide
an additional value to the option, instead of overriding it completely.

This changes the patch so that callers provide an additional value to
the diagnostic. This value is emitted as the suffix "=value" when
displaying the flag name.

[rsmith]

LGTM

include/clang/Basic/Diagnostic.h

private:
   unsigned DelayedDiagID;
 
   /// \brief First string argument for the delayed diagnostic.
   std::string DelayedDiagArg1;
 
   /// \brief Second string argument for the delayed diagnostic.
   std::string DelayedDiagArg2;
 
+  /// \brief Flag name value.
+  ///
+  /// Some flags accept values. For instance, -Wframe-larger-than or -Rpass.
+  /// When reporting a diagnostic with those flags, it is useful to also
+  /// report the value that actually triggered the flag. The content of this
+  /// string is a value to be emitted after the flag name.
+  std::string FlagNameValue;
+
 public:
   explicit DiagnosticsEngine(
                       const IntrusiveRefCntPtr<DiagnosticIDs> &Diags,
                       DiagnosticOptions *DiagOpts,
                       DiagnosticConsumer *client = 0,
                       bool ShouldOwnClient = true);
   ~DiagnosticsEngine();
 
public:
   /// \brief Issue the message to the client.
   ///
   /// This actually returns an instance of DiagnosticBuilder which emits the
   /// diagnostics (through @c ProcessDiag) when it is destroyed.
   ///
   /// \param DiagID A member of the @c diag::kind enum.
   /// \param Loc Represents the source location associated with the diagnostic,
   /// which can be an invalid location if no position information is available.
+  /// \param FlagNameValue A string that represents the value that triggered
+  /// this diagnostic. If given, this value will be emitted as "=value"
+  /// after the flag name.
+  inline DiagnosticBuilder Report(SourceLocation Loc, unsigned DiagID,
+                                  StringRef Val);
   inline DiagnosticBuilder Report(SourceLocation Loc, unsigned DiagID);
   inline DiagnosticBuilder Report(unsigned DiagID);
 
   void Report(const StoredDiagnostic &storedDiag);
 
   /// \brief Determine whethere there is already a diagnostic in flight.
   bool isDiagnosticInFlight() const { return CurDiagID != ~0U; }
 
public:
   /// diagnostic. A copy of this string will be stored in the
   /// DiagnosticsEngine object itself.
   void SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1 = "",
                             StringRef Arg2 = "");
   
   /// \brief Clear out the current diagnostic.
   void Clear() { CurDiagID = ~0U; }
 
+  /// \brief Return the overridden name for this diagnostic flag.
+  StringRef getFlagNameValue() const { return StringRef(FlagNameValue); }
+
 private:
   /// \brief Report the delayed diagnostic.
   void ReportDelayed();
 
   // This is private state used by DiagnosticBuilder.  We put it here instead of
   // in DiagnosticBuilder in order to keep DiagnosticBuilder a small lightweight
   // object.  This implementation choice means that we can only have one
   // diagnostic "in flight" at a time, but this seems to be a reasonable

 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            const FixItHint &Hint) {
   if (!Hint.isNull())
     DB.AddFixItHint(Hint);
   return DB;
 }
 
-inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc,
-                                                   unsigned DiagID) {
+inline DiagnosticBuilder
+DiagnosticsEngine::Report(SourceLocation Loc, unsigned DiagID, StringRef Val) {
   assert(CurDiagID == ~0U && "Multiple diagnostics in flight at once!");
   CurDiagLoc = Loc;
   CurDiagID = DiagID;
+  FlagNameValue = Val.str();
   return DiagnosticBuilder(this);
 }
+
+inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc,
+                                                   unsigned DiagID) {
+  return Report(Loc, DiagID, "");
+}
+
 inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
-  return Report(SourceLocation(), DiagID);
+  return Report(SourceLocation(), DiagID, "");
 }
 
 //===----------------------------------------------------------------------===//
 // Diagnostic
 //===----------------------------------------------------------------------===//
 
 /// A little helper class (which is basically a smart pointer that forwards
 /// info from DiagnosticsEngine) that allows clients to enquire about the

lib/CodeGen/CodeGenAction.cpp

if (CodeGenOpts.OptimizationRemarkPattern &&
     if (Line > 0) {
       // If -gcolumn-info was not used, Column will be 0. This upsets the
       // source manager, so if Column is not set, set it to 1.
       if (Column == 0)
         Column = 1;
       Loc = SourceMgr.translateFileLineCol(FileMgr.getFile(Filename), Line,
                                            Column);
     }
-    Diags.Report(Loc, diag::remark_fe_backend_optimization_remark)
+    Diags.Report(Loc, diag::remark_fe_backend_optimization_remark,
+                 D.getPassName())
         << D.getMsg().str();
 
     if (Line == 0)
       // If we could not extract a source location for the diagnostic,
       // inform the user how they can get source locations back.
       //
       // FIXME: We should really be generating !srcloc annotations when
       // -Rpass is used. !srcloc annotations need to be emitted in

lib/Frontend/TextDiagnosticPrinter.cpp

if (Level == DiagnosticsEngine::Error &&
       OS << " [-Werror";
       Started = true;
     }
 
     StringRef Opt = DiagnosticIDs::getWarningOptionForDiag(Info.getID());
     if (!Opt.empty()) {
       OS << (Started ? "," : " [")
          << (DiagnosticIDs::isRemark(Info.getID()) ? "-R" : "-W") << Opt;
+      StringRef OptValue = Info.getDiags()->getFlagNameValue();
+      if (!OptValue.empty())
+        OS << "=" << OptValue;
       Started = true;
     }
   }
 
   // If the user wants to see category information, include it too.
   if (DiagOpts.ShowCategories) {
     unsigned DiagCategory =
       DiagnosticIDs::getCategoryNumberForDiag(Info.getID());

test/Frontend/optimization-remark.c

 // This file tests the -Rpass= flag with the inliner. The test is
 // designed to always trigger the inliner, so it should be independent
 // of the optimization level.
 
 // RUN: %clang -c %s -Rpass=inline -O0 -gline-tables-only -S -o /dev/null 2> %t.err
 // RUN: FileCheck < %t.err %s --check-prefix=INLINE
 
 // RUN: %clang -c %s -Rpass=inline -O0 -S -o /dev/null 2> %t.err
 // RUN: FileCheck < %t.err %s --check-prefix=INLINE-NO-LOC
 
 int foo(int x, int y) __attribute__((always_inline));
 
 int foo(int x, int y) { return x + y; }
 int bar(int j) { return foo(j, j - 2); }
 
-// INLINE: remark: foo inlined into bar [-Rpass]
+// INLINE: remark: foo inlined into bar [-Rpass=inline]
 
 // INLINE-NO-LOC: {{^remark: foo inlined into bar}}
 // INLINE-NO-LOC: note: use -gline-tables-only -gcolumn-info to track