[driver/diagnostics] init color earlier, add color to driver
diff mbox

Message ID CAESRpQA5BYi1+ZYomGFwAMkY+6n7YbKPQS1YsC=C3u5A2DZZpw@mail.gmail.com
State New
Headers show

Commit Message

Manuel López-Ibáñez Dec. 1, 2014, 11:40 p.m. UTC
The initialization of color diagnostics occurs too late to affect, for
example, the "unrecognized command line option" error.  Unfortunately,
in the compiler proper we cannot initialize colors just after
diagnostic_initialize() because the FEs replace the pretty-printer
with their own by destroying the existing one and creating a new one
from scratch, thus losing all settings.  (I must admit I tried to
construct an object from its base, but it didn't work.) That is why
colors are initialized after language specific diagnostic initialize
(but still before options parsing).

Fortunately, in the driver, we can do it just as it should be.

Bootstrapped and regression tested. Tested that it works by doing:

xgcc -B./ /dev/null -test
xgcc -fdiagnostics-color=never -B./ /dev/null -test
cc1 -B./ /dev/null -test
cc1 -fdiagnostics-color=never -B./ /dev/null -test

OK?

gcc/ChangeLog:

2014-12-02  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    * diagnostic.c (diagnostic_color_init): New.
    * diagnostic.h: Declare.
    * gcc.c (driver::global_initializations): Use it.
    (driver_handle_option): Handle -fdiagnostics-color_.
    * toplev.c: Do not include diagnostic-color.h.
    (process_options): Do not initialize color diagnostics here.
    * common.opt (fdiagnostics-color=): Add Driver.
    * opts-global.c (init_options_once): Initialize color here.


A possible improvement (in my opinion) would be to have

void
diagnostic_color_init (diagnostic_context *context,
                       int value = DIAGNOSTICS_COLOR_DEFAULT)
{
  /* If DIAGNOSTICS_COLOR_DEFAULT is -1, default to -fdiagnostics-color=auto
     if GCC_COLORS is in the environment, otherwise default to
     -fdiagnostics-color=never, for other values default to that
     -fdiagnostics-color={never,auto,always}.  */
  switch (value)
    {
    case -1:
      if (!getenv ("GCC_COLORS"))
        break;
      value = DIAGNOSTICS_COLOR_AUTO;
      /* FALLTHRU */
   default:
      pp_show_color (context->printer)
       = colorize_init ((diagnostic_color_rule_t) value);
      break;
    }
}

and then we could simply have in the options handling:

    case OPT_fdiagnostics_color_:
      diagnostic_color_init (dc, value);
      break;

This hides some details within diagnostic.c, but I'm not sure what is
the current policy about default arguments.

Comments

Joseph Myers Dec. 2, 2014, 11:01 p.m. UTC | #1
On Tue, 2 Dec 2014, Manuel López-Ibáñez wrote:

> 2014-12-02  Manuel López-Ibáñez  <manu@gcc.gnu.org>
> 
>     * diagnostic.c (diagnostic_color_init): New.
>     * diagnostic.h: Declare.
>     * gcc.c (driver::global_initializations): Use it.
>     (driver_handle_option): Handle -fdiagnostics-color_.
>     * toplev.c: Do not include diagnostic-color.h.
>     (process_options): Do not initialize color diagnostics here.
>     * common.opt (fdiagnostics-color=): Add Driver.
>     * opts-global.c (init_options_once): Initialize color here.

The gcc.c / common.opt / opts-global.c changes are OK.
Jakub Jelinek Dec. 3, 2014, 7:59 a.m. UTC | #2
On Tue, Dec 02, 2014 at 12:40:12AM +0100, Manuel López-Ibáñez wrote:
> 2014-12-02  Manuel López-Ibáñez  <manu@gcc.gnu.org>
> 
>     * diagnostic.c (diagnostic_color_init): New.
>     * diagnostic.h: Declare.
>     * gcc.c (driver::global_initializations): Use it.
>     (driver_handle_option): Handle -fdiagnostics-color_.
>     * toplev.c: Do not include diagnostic-color.h.
>     (process_options): Do not initialize color diagnostics here.
>     * common.opt (fdiagnostics-color=): Add Driver.
>     * opts-global.c (init_options_once): Initialize color here.
> 
> 
> A possible improvement (in my opinion) would be to have
> 
> void
> diagnostic_color_init (diagnostic_context *context,
>                        int value = DIAGNOSTICS_COLOR_DEFAULT)
> {
>   /* If DIAGNOSTICS_COLOR_DEFAULT is -1, default to -fdiagnostics-color=auto
>      if GCC_COLORS is in the environment, otherwise default to
>      -fdiagnostics-color=never, for other values default to that
>      -fdiagnostics-color={never,auto,always}.  */
>   switch (value)
>     {
>     case -1:
>       if (!getenv ("GCC_COLORS"))
>         break;
>       value = DIAGNOSTICS_COLOR_AUTO;
>       /* FALLTHRU */
>    default:
>       pp_show_color (context->printer)
>        = colorize_init ((diagnostic_color_rule_t) value);
>       break;
>     }
> }

I think using a default argument for this is fine, though of course
you need to declare the default argument in the header containing
the prototype, not in the function definition.

Ok with that change.

	Jakub

Patch
diff mbox

Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 218192)
+++ gcc/diagnostic.c	(working copy)
@@ -153,10 +153,39 @@  diagnostic_initialize (diagnostic_contex
   context->x_data = NULL;
   context->lock = 0;
   context->inhibit_notes_p = false;
 }
 
+/* Maybe initialize the color support. We require clients to do this
+   explicitly.  */
+
+void
+diagnostic_color_init (diagnostic_context *context)
+{
+  /* If DIAGNOSTICS_COLOR_DEFAULT is -1, default to -fdiagnostics-color=auto
+     if GCC_COLORS is in the environment, otherwise default to
+     -fdiagnostics-color=never, for other values default to that
+     -fdiagnostics-color={never,auto,always}.  */
+  switch ((int) DIAGNOSTICS_COLOR_DEFAULT)
+    {
+    case -1:
+      if (!getenv ("GCC_COLORS"))
+	break;
+      /* FALLTHRU */
+    case DIAGNOSTICS_COLOR_AUTO:
+      pp_show_color (context->printer)
+	= colorize_init (DIAGNOSTICS_COLOR_AUTO);
+      break;
+    case DIAGNOSTICS_COLOR_YES:
+      pp_show_color (context->printer)
+	= colorize_init (DIAGNOSTICS_COLOR_YES);
+      break;
+    default:
+      break;
+    }
+}
+
 /* Do any cleaning up required after the last diagnostic is emitted.  */
 
 void
 diagnostic_finish (diagnostic_context *context)
 {
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 218192)
+++ gcc/diagnostic.h	(working copy)
@@ -264,10 +264,11 @@  extern diagnostic_context *global_dc;
 #define diagnostic_override_option_index(DI, OPTIDX) \
     ((DI)->option_index = (OPTIDX))
 
 /* Diagnostic related functions.  */
 extern void diagnostic_initialize (diagnostic_context *, int);
+extern void diagnostic_color_init (diagnostic_context *);
 extern void diagnostic_finish (diagnostic_context *);
 extern void diagnostic_report_current_module (diagnostic_context *, location_t);
 extern void diagnostic_show_locus (diagnostic_context *, const diagnostic_info *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 218192)
+++ gcc/gcc.c	(working copy)
@@ -36,10 +36,11 @@  compilation is specified by a string cal
 #include "obstack.h"
 #include "intl.h"
 #include "prefix.h"
 #include "gcc.h"
 #include "diagnostic.h"
+#include "diagnostic-color.h"
 #include "flags.h"
 #include "opts.h"
 #include "params.h"
 #include "vec.h"
 #include "filenames.h"
@@ -3606,10 +3607,15 @@  driver_handle_option (struct gcc_options
       else
 	compare_debug_opt = arg;
       save_switch (compare_debug_replacement_opt, 0, NULL, validated, true);
       return true;
 
+    case OPT_fdiagnostics_color_:
+      pp_show_color (dc->printer)
+	= colorize_init ((diagnostic_color_rule_t) value);
+      break;
+
     case OPT_Wa_:
       {
 	int prev, j;
 	/* Pass the rest of this option to the assembler.  */
 
@@ -6973,10 +6979,11 @@  driver::global_initializations ()
   unlock_std_streams ();
 
   gcc_init_libintl ();
 
   diagnostic_initialize (global_dc, 0);
+  diagnostic_color_init (global_dc);
 
 #ifdef GCC_DRIVER_HOST_INITIALIZATION
   /* Perform host dependent initialization when needed.  */
   GCC_DRIVER_HOST_INITIALIZATION;
 #endif
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c	(revision 218192)
+++ gcc/toplev.c	(working copy)
@@ -84,11 +84,10 @@  along with GCC; see the file COPYING3.  
 #include "tree-ssa-alias.h"
 #include "internal-fn.h"
 #include "gimple-expr.h"
 #include "gimple.h"
 #include "plugin.h"
-#include "diagnostic-color.h"
 #include "context.h"
 #include "pass_manager.h"
 #include "auto-profile.h"
 #include "dwarf2out.h"
 #include "bitmap.h"
@@ -1266,33 +1266,10 @@  process_options (void)
      This can happen with incorrect pre-processed input. */
   debug_hooks = &do_nothing_debug_hooks;
 
   maximum_field_alignment = initial_max_fld_align * BITS_PER_UNIT;
 
-  /* If DIAGNOSTICS_COLOR_DEFAULT is -1, default to -fdiagnostics-color=auto
-     if GCC_COLORS is in the environment, otherwise default to
-     -fdiagnostics-color=never, for other values default to that
-     -fdiagnostics-color={never,auto,always}.  */
-  if (!global_options_set.x_flag_diagnostics_show_color)
-    switch ((int) DIAGNOSTICS_COLOR_DEFAULT)
-      {
-      case -1:
-	if (!getenv ("GCC_COLORS"))
-	  break;
-	/* FALLTHRU */
-      case DIAGNOSTICS_COLOR_AUTO:
-	pp_show_color (global_dc->printer)
-	  = colorize_init (DIAGNOSTICS_COLOR_AUTO);
-	break;
-      case DIAGNOSTICS_COLOR_YES:
-	pp_show_color (global_dc->printer)
-	  = colorize_init (DIAGNOSTICS_COLOR_YES);
-	break;
-      default:
-	break;
-      }
-
   /* Allow the front end to perform consistency checks and do further
      initialization based on the command line options.  This hook also
      sets the original filename if appropriate (e.g. foo.i -> foo.c)
      so we can correctly initialize debug output.  */
   no_backend = lang_hooks.post_options (&main_input_filename);
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 218192)
+++ gcc/common.opt	(working copy)
@@ -1094,11 +1094,11 @@  Show the source line with a caret indica
 fdiagnostics-color
 Common Alias(fdiagnostics-color=,always,never)
 ;
 
 fdiagnostics-color=
-Common Joined RejectNegative Var(flag_diagnostics_show_color) Enum(diagnostic_color_rule) Init(DIAGNOSTICS_COLOR_NO)
+Driver Common Joined RejectNegative Var(flag_diagnostics_show_color) Enum(diagnostic_color_rule) Init(DIAGNOSTICS_COLOR_NO)
 -fdiagnostics-color=[never|always|auto]	Colorize diagnostics
 
 ; Required for these enum values.
 SourceInclude
 diagnostic-color.h
Index: gcc/opts-global.c
===================================================================
--- gcc/opts-global.c	(revision 218192)
+++ gcc/opts-global.c	(working copy)
@@ -259,10 +259,15 @@  init_options_once (void)
 {
   /* Perform language-specific options initialization.  */
   initial_lang_mask = lang_hooks.option_lang_mask ();
 
   lang_hooks.initialize_diagnostics (global_dc);
+  /* ??? Ideally, we should do this earlier and the FEs will override
+     it if desired (none do it so far).  However, the way the FEs
+     construct their pretty-printers means that all previous settings
+     are overriden.  */
+  diagnostic_color_init (global_dc);
 }
 
 /* Decode command-line options to an array, like
    decode_cmdline_options_to_array and with the same arguments but
    using the default lang_mask.  */