Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Bruno Cardoso Lopes via cfe-commits]

bruno closed this revision.
bruno added a comment.

Committed in r271042


http://reviews.llvm.org/D20404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Duncan P. N. Exon Smith via cfe-commits]

LGTM.

> On 2016-May-26, at 11:34, Bruno Cardoso Lopes  wrote:
> 
> bruno added a comment.
> 
> Ping!
> 
> 
> http://reviews.llvm.org/D20404
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Duncan P. N. Exon Smith via cfe-commits]

dexonsmith added a subscriber: dexonsmith.
dexonsmith added a comment.

LGTM.


http://reviews.llvm.org/D20404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Bruno Cardoso Lopes via cfe-commits]

bruno added a comment.

Ping!


http://reviews.llvm.org/D20404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Bruno Cardoso Lopes via cfe-commits]

bruno added a comment.

Ping!


http://reviews.llvm.org/D20404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Bruno Cardoso Lopes via cfe-commits]

bruno added reviewers: dexonsmith, silvas.
bruno removed subscribers: silvas, dexonsmith.
bruno updated this revision to Diff 57993.
bruno added a comment.

Great idea Duncan, attached a new patch!


http://reviews.llvm.org/D20404

Files:
  include/clang/Frontend/CompilerInvocation.h
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -855,8 +855,51 @@
 ModuleFiles.end());
 }
 
+static bool parseShowColorsArgs(const ArgList , bool DefaultColor) {
+  // Color diagnostics default to auto ("on" if terminal supports) in the driver
+  // but default to off in cc1, needing an explicit OPT_fdiagnostics_color.
+  // Support both clang's -f[no-]color-diagnostics and gcc's
+  // -f[no-]diagnostics-colors[=never|always|auto].
+  enum {
+Colors_On,
+Colors_Off,
+Colors_Auto
+  } ShowColors = DefaultColor ? Colors_Auto : Colors_Off;
+  for (Arg *A : Args) {
+const Option  = A->getOption();
+if (!O.matches(options::OPT_fcolor_diagnostics) &&
+!O.matches(options::OPT_fdiagnostics_color) &&
+!O.matches(options::OPT_fno_color_diagnostics) &&
+!O.matches(options::OPT_fno_diagnostics_color) &&
+!O.matches(options::OPT_fdiagnostics_color_EQ))
+  continue;
+
+if (O.matches(options::OPT_fcolor_diagnostics) ||
+O.matches(options::OPT_fdiagnostics_color)) {
+  ShowColors = Colors_On;
+} else if (O.matches(options::OPT_fno_color_diagnostics) ||
+   O.matches(options::OPT_fno_diagnostics_color)) {
+  ShowColors = Colors_Off;
+} else {
+  assert(O.matches(options::OPT_fdiagnostics_color_EQ));
+  StringRef Value(A->getValue());
+  if (Value == "always")
+ShowColors = Colors_On;
+  else if (Value == "never")
+ShowColors = Colors_Off;
+  else if (Value == "auto")
+ShowColors = Colors_Auto;
+}
+  }
+  if (ShowColors == Colors_On ||
+  (ShowColors == Colors_Auto && llvm::sys::Process::StandardErrHasColors()))
+return true;
+  return false;
+}
+
 bool clang::ParseDiagnosticArgs(DiagnosticOptions , ArgList ,
-DiagnosticsEngine *Diags) {
+DiagnosticsEngine *Diags,
+bool DefaultDiagColor) {
   using namespace options;
   bool Success = true;
 
@@ -869,7 +912,7 @@
   Opts.Pedantic = Args.hasArg(OPT_pedantic);
   Opts.PedanticErrors = Args.hasArg(OPT_pedantic_errors);
   Opts.ShowCarets = !Args.hasArg(OPT_fno_caret_diagnostics);
-  Opts.ShowColors = Args.hasArg(OPT_fcolor_diagnostics);
+  Opts.ShowColors = parseShowColorsArgs(Args, DefaultDiagColor);
   Opts.ShowColumn = Args.hasFlag(OPT_fshow_column,
  OPT_fno_show_column,
  /*Default=*/true);
@@ -2226,7 +2269,8 @@
   Success &= ParseAnalyzerArgs(*Res.getAnalyzerOpts(), Args, Diags);
   Success &= ParseMigratorArgs(Res.getMigratorOpts(), Args);
   ParseDependencyOutputArgs(Res.getDependencyOutputOpts(), Args);
-  Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, );
+  Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, ,
+ false /*DefaultDiagColor*/);
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   ParseFileSystemArgs(Res.getFileSystemOpts(), Args);
   // FIXME: We shouldn't have to pass the DashX option around here
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -5529,43 +5529,27 @@
   CmdArgs.push_back("-fno-diagnostics-show-note-include-stack");
   }
 
-  // Color diagnostics are the default, unless the terminal doesn't support
-  // them.
-  // Support both clang's -f[no-]color-diagnostics and gcc's
-  // -f[no-]diagnostics-colors[=never|always|auto].
-  enum { Colors_On, Colors_Off, Colors_Auto } ShowColors = Colors_Auto;
-  for (const auto  : Args) {
-const Option  = Arg->getOption();
+  // Color diagnostics are parsed by the driver directly from argv
+  // and later re-parsed to construct this job; claim any possible
+  // color diagnostic here to avoid warn_drv_unused_argument and
+  // diagnose bad OPT_fdiagnostics_color_EQ values.
+  for (Arg *A : Args) {
+const Option  = A->getOption();
 if (!O.matches(options::OPT_fcolor_diagnostics) &&
 !O.matches(options::OPT_fdiagnostics_color) &&
 !O.matches(options::OPT_fno_color_diagnostics) &&
 !O.matches(options::OPT_fno_diagnostics_color) &&
 !O.matches(options::OPT_fdiagnostics_color_EQ))
   continue;
-
-Arg->claim();
-if (O.matches(options::OPT_fcolor_diagnostics) ||
-O.matches(options::OPT_fdiagnostics_color)) {
-  ShowColors = Colors_On;
-} else if 

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Duncan P. N. Exon Smith via cfe-commits]

I don't think we even need a helper.

Clang::ConstructJob takes a `Compilation`, which has a `Driver`, which has a 
`DiagnosticsEngine`, which has a `DiagnosticsOptions`.

In other words, I think you can delete the code in Clang::ConstructJob (change 
it to a lookup) now that you've changed clang::ParseDiagnosticArgs to update 
`DiagnosticsOptions`.

> On 2016-May-18, at 18:54, Sean Silva  wrote:
> 
> silvas added a subscriber: silvas.
> silvas added a comment.
> 
> I don't see an issue with putthing this as a helper in libBasic. We may need 
> to add a libOption dependency to it but that sounds fine.
> 
> 
> http://reviews.llvm.org/D20404
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Sean Silva via cfe-commits]

silvas added a subscriber: silvas.
silvas added a comment.

I don't see an issue with putthing this as a helper in libBasic. We may need to 
add a libOption dependency to it but that sounds fine.


http://reviews.llvm.org/D20404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D20404: [Driver] Fix driver support for color diagnostics

[Bruno Cardoso Lopes via cfe-commits]

bruno created this revision.
bruno added a reviewer: rsmith.
bruno added subscribers: cfe-commits, dexonsmith.

Diagnostics that happen during driver time do not have color output
support unless -fcolor-diagonostic is explicitly passed into the driver.
OTOH, it works great for cc1 mode since dianostic arguments are properly
handled and color is enabled by default if the terminal supports it.

This patch fix this behavior by using the same logic present in -cc1
argument construction to decide whether it should use colors. I tried to
find a way to refactor this logic into a common place that could be used
from both lib/Driver/Tools.cpp and lib/Frontend/CompilerInvocation.cpp,
but could not see any options besides libBasic, which also seems wrong
because it's not the ideal place to put options parsing code either. So
I duplicated part of the logic and changed some driver unrelated logic.
(look for ShowColors in lib/Driver/Tools.cpp for details). Ideas welcome.

I do not see a way to write a testcase for this. 


http://reviews.llvm.org/D20404

Files:
  lib/Frontend/CompilerInvocation.cpp

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -855,6 +855,44 @@
 ModuleFiles.end());
 }
 
+static bool parseShowColorsArgs(const ArgList ) {
+  // Color diagnostics are the default, unless the terminal doesn't support
+  // them.
+  // Support both clang's -f[no-]color-diagnostics and gcc's
+  // -f[no-]diagnostics-colors[=never|always|auto].
+  enum { Colors_On, Colors_Off, Colors_Auto } ShowColors = Colors_Auto;
+  for (const auto  : Args) {
+const Option  = Arg->getOption();
+if (!O.matches(options::OPT_fcolor_diagnostics) &&
+!O.matches(options::OPT_fdiagnostics_color) &&
+!O.matches(options::OPT_fno_color_diagnostics) &&
+!O.matches(options::OPT_fno_diagnostics_color) &&
+!O.matches(options::OPT_fdiagnostics_color_EQ))
+  continue;
+
+if (O.matches(options::OPT_fcolor_diagnostics) ||
+O.matches(options::OPT_fdiagnostics_color)) {
+  ShowColors = Colors_On;
+} else if (O.matches(options::OPT_fno_color_diagnostics) ||
+   O.matches(options::OPT_fno_diagnostics_color)) {
+  ShowColors = Colors_Off;
+} else {
+  assert(O.matches(options::OPT_fdiagnostics_color_EQ));
+  StringRef Value(Arg->getValue());
+  if (Value == "always")
+ShowColors = Colors_On;
+  else if (Value == "auto")
+ShowColors = Colors_Auto;
+  else // Value == "never" or any other value
+ShowColors = Colors_Off;
+}
+  }
+  if (ShowColors == Colors_On ||
+  (ShowColors == Colors_Auto && 
llvm::sys::Process::StandardErrHasColors()))
+return true;
+  return false;
+}
+
 bool clang::ParseDiagnosticArgs(DiagnosticOptions , ArgList ,
 DiagnosticsEngine *Diags) {
   using namespace options;
@@ -869,7 +907,7 @@
   Opts.Pedantic = Args.hasArg(OPT_pedantic);
   Opts.PedanticErrors = Args.hasArg(OPT_pedantic_errors);
   Opts.ShowCarets = !Args.hasArg(OPT_fno_caret_diagnostics);
-  Opts.ShowColors = Args.hasArg(OPT_fcolor_diagnostics);
+  Opts.ShowColors = parseShowColorsArgs(Args);
   Opts.ShowColumn = Args.hasFlag(OPT_fshow_column,
  OPT_fno_show_column,
  /*Default=*/true);


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -855,6 +855,44 @@
 ModuleFiles.end());
 }
 
+static bool parseShowColorsArgs(const ArgList ) {
+  // Color diagnostics are the default, unless the terminal doesn't support
+  // them.
+  // Support both clang's -f[no-]color-diagnostics and gcc's
+  // -f[no-]diagnostics-colors[=never|always|auto].
+  enum { Colors_On, Colors_Off, Colors_Auto } ShowColors = Colors_Auto;
+  for (const auto  : Args) {
+const Option  = Arg->getOption();
+if (!O.matches(options::OPT_fcolor_diagnostics) &&
+!O.matches(options::OPT_fdiagnostics_color) &&
+!O.matches(options::OPT_fno_color_diagnostics) &&
+!O.matches(options::OPT_fno_diagnostics_color) &&
+!O.matches(options::OPT_fdiagnostics_color_EQ))
+  continue;
+
+if (O.matches(options::OPT_fcolor_diagnostics) ||
+O.matches(options::OPT_fdiagnostics_color)) {
+  ShowColors = Colors_On;
+} else if (O.matches(options::OPT_fno_color_diagnostics) ||
+   O.matches(options::OPT_fno_diagnostics_color)) {
+  ShowColors = Colors_Off;
+} else {
+  assert(O.matches(options::OPT_fdiagnostics_color_EQ));
+  StringRef Value(Arg->getValue());
+  if (Value == "always")
+ShowColors = Colors_On;
+  else if (Value == "auto")
+ShowColors = Colors_Auto;
+