[PATCH] Allow overriding flag names in diagnostics.

[Alp Toker]

On 22/04/2014 00:03, Diego Novillo wrote:
>    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.
>
> Hi rsmith,
>
> http://reviews.llvm.org/D3441
>
> CHANGE SINCE LAST DIFF
>    http://reviews.llvm.org/D3441?vs=8703&id=8707#toc
>
> Files:
>    include/clang/Basic/Diagnostic.h
>    lib/CodeGen/CodeGenAction.cpp
>    lib/Frontend/TextDiagnosticPrinter.cpp
>    test/Frontend/optimization-remark.c
>
> D3441.2.patch
>
>
> Index: include/clang/Basic/Diagnostic.h
> ===================================================================
> --- include/clang/Basic/Diagnostic.h
> +++ include/clang/Basic/Diagnostic.h
> @@ -341,6 +341,14 @@
>     /// \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,
> @@ -646,6 +654,11 @@
>     /// \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);
>   
> @@ -681,6 +694,9 @@
>     /// \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();
> @@ -1084,15 +1100,22 @@
>     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);
>   }

Did you consider adding a DiagnosticBuilder function to append a 
FlagNameValue instead of all this?

That would reduce the impact of your patch and improve readability in 
the callers. Something like:

   Diag(Loc, ID).extendFlag("=mypass") << D;

Alp.


> +
> +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, "");
>   }
>   
>   //===----------------------------------------------------------------------===//
> Index: lib/CodeGen/CodeGenAction.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenAction.cpp
> +++ lib/CodeGen/CodeGenAction.cpp
> @@ -404,7 +404,8 @@
>         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)
> Index: lib/Frontend/TextDiagnosticPrinter.cpp
> ===================================================================
> --- lib/Frontend/TextDiagnosticPrinter.cpp
> +++ lib/Frontend/TextDiagnosticPrinter.cpp
> @@ -83,6 +83,9 @@
>       if (!Opt.empty()) {
>         OS << (Started ? "," : " [")
>            << (DiagnosticIDs::isRemark(Info.getID()) ? "-R" : "-W") << Opt;
> +      StringRef OptValue = Info.getDiags()->getFlagNameValue();
> +      if (!OptValue.empty())
> +        OS << "=" << OptValue;
>         Started = true;
>       }
>     }
> Index: test/Frontend/optimization-remark.c
> ===================================================================
> --- test/Frontend/optimization-remark.c
> +++ test/Frontend/optimization-remark.c
> @@ -13,7 +13,7 @@
>   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
>
>
> _______________________________________________
> 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