[PATCH diagnostics] PR 53061 cleanup initialization

[PATCH diagnostics] PR 53061 cleanup initialization

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

This is an old patch of mine that never got finished. I updated it following
the suggestions of Gabriel here
https://gcc.gnu.org/ml/gcc-patches/2012-04/msg00443.html

In cp/error.c, I separate the initialization of the diagnostic context
from the initialization of the scratch pretty-printer (cxx_pp).  This
was suggested by Gabriel in the last review of the patch and now I
realize it was a good idea. Now cxx_initialize_diagnostics is the
equivalent version of c_initialize_diagnostics.  To avoid having to
make extern a bunch of functions, I moved it from cp-objcp-common.c to
error.c.

I moved the setting of diagnostic_format_decoder (global_dc) =
c_tree_printer, from c_objc_common_init to c_initialize_diagnostics,
and right after c_common_diagnostics_defaults.  This
mimics what is done in cxx_initialize_diagnostics.

Moreover, in both c_initialize_diagnostics and
cxx_initialize_diagnostics, the FE-specific pretty-printer is
initialized first and then other settings are applied. This does not
make a difference right now, but if in the future one wishes to touch
something in the pretty-printer, it will not get overriden
immediately.

In fact, the code I removed in c_common_initialize_diagnostics, which
sets line_cutoff to 80, is useless because the pretty-printer created
here is actually never used, but overriden by the FE-specific
pretty-printers. This also means that doc/invoke.texi was wrong.

Finally, it is useless to set the maximum line length to 0 in the
constructor of cxx_pretty_printer.  It is the default anyway.

Bootstrapped and tested on x86_64-linux-gnu.

OK?

gcc/c/ChangeLog:

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

    PR c++/53061
    * c-objc-common.c (c_objc_common_init): Do not do diagnostics
    initialization here...
    (c_initialize_diagnostics): ... but here. Set defaults after
    building pretty-printer.

gcc/ChangeLog:

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

    PR c++/53061
    * doc/invoke.texi (fmessage-length): Update text to match reality.

gcc/cp/ChangeLog:

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

    PR c++/53061
    * cp-objcp-common.c: Do not include new.
    (cxx_initialize_diagnostics): Move from here to ...
    * error.c (cxx_initialize_diagnostics): : ... here. Move
    diagnostics initialization here from init_error.
    (cxx_pp): Use a real pointer not a macro.
    (init_error): Just initialize cxx_pp.
    * cxx-pretty-print.c (cxx_pretty_printer::cxx_pretty_printer): Do
    not set maximum line length.

gcc/c-family/ChangeLog:

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

    PR c++/53061
    * c-opts.c (c_common_diagnostics_defaults): Renamed from
    c_common_initialize_diagnostics.
    * c-common.h: Likewise.

[-- Attachment #2: pr530631.diff --]
[-- Type: text/plain, Size: 9346 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 215890)
+++ gcc/doc/invoke.texi	(working copy)
@@ -3075,15 +3075,14 @@ information should be reported.  Note th
 honor these options.
 
 @table @gcctabopt
 @item -fmessage-length=@var{n}
 @opindex fmessage-length
-Try to format error messages so that they fit on lines of about @var{n}
-characters.  The default is 72 characters for @command{g++} and 0 for the rest of
-the front ends supported by GCC@.  If @var{n} is zero, then no
-line-wrapping is done; each error message appears on a single
-line.
+Try to format error messages so that they fit on lines of about
+@var{n} characters.  If @var{n} is zero, then no line-wrapping will be
+done; each error message will appear on a single line.  This is the
+default for all front ends.
 
 @item -fdiagnostics-show-location=once
 @opindex fdiagnostics-show-location
 Only meaningful in line-wrapping mode.  Instructs the diagnostic messages
 reporter to emit source location information @emph{once}; that is, in
Index: gcc/c-family/c-opts.c
===================================================================
--- gcc/c-family/c-opts.c	(revision 215890)
+++ gcc/c-family/c-opts.c	(working copy)
@@ -176,25 +176,14 @@ c_diagnostic_finalizer (diagnostic_conte
   virt_loc_aware_diagnostic_finalizer (context, diagnostic);
   pp_destroy_prefix (context->printer);
   pp_newline_and_flush (context->printer);
 }
 
-/* Common diagnostics initialization.  */
+/* Common default settings for diagnostics.  */
 void
-c_common_initialize_diagnostics (diagnostic_context *context)
+c_common_diagnostics_defaults (diagnostic_context *context)
 {
-  /* This is conditionalized only because that is the way the front
-     ends used to do it.  Maybe this should be unconditional?  */
-  if (c_dialect_cxx ())
-    {
-      /* By default wrap lines at 80 characters.  Is getenv
-	 ("COLUMNS") preferable?  */
-      diagnostic_line_cutoff (context) = 80;
-      /* By default, emit location information once for every
-	 diagnostic message.  */
-      diagnostic_prefixing_rule (context) = DIAGNOSTICS_SHOW_PREFIX_ONCE;
-    }
   diagnostic_finalizer (context) = c_diagnostic_finalizer;
   context->opt_permissive = OPT_fpermissive;
 }
 
 /* Whether options from all C-family languages should be accepted
Index: gcc/c-family/c-common.h
===================================================================
--- gcc/c-family/c-common.h	(revision 215890)
+++ gcc/c-family/c-common.h	(working copy)
@@ -824,11 +824,11 @@ extern void set_compound_literal_name (t
 
 extern tree build_va_arg (location_t, tree, tree);
 
 extern const unsigned int c_family_lang_mask;
 extern unsigned int c_common_option_lang_mask (void);
-extern void c_common_initialize_diagnostics (diagnostic_context *);
+extern void c_common_diagnostics_defaults (diagnostic_context *);
 extern bool c_common_complain_wrong_lang_p (const struct cl_option *);
 extern void c_common_init_options_struct (struct gcc_options *);
 extern void c_common_init_options (unsigned int, struct cl_decoded_option *);
 extern bool c_common_post_options (const char **);
 extern bool c_common_init (void);
Index: gcc/c/c-objc-common.c
===================================================================
--- gcc/c/c-objc-common.c	(revision 215890)
+++ gcc/c/c-objc-common.c	(working copy)
@@ -60,19 +60,11 @@ c_warn_unused_global_decl (const_tree de
 bool
 c_objc_common_init (void)
 {
   c_init_decl_processing ();
 
-  if (c_common_init () == false)
-    return false;
-
-  /* These were not defined in the Objective-C front end, but I'm
-     putting them here anyway.  The diagnostic format decoder might
-     want an enhanced ObjC implementation.  */
-  diagnostic_format_decoder (global_dc) = &c_tree_printer;
-
-  return true;
+  return c_common_init ();
 }
 
 /* Called during diagnostic message formatting process to print a
    source-level entity onto BUFFER.  The meaning of the format specifiers
    is as follows:
@@ -184,19 +176,20 @@ has_c_linkage (const_tree decl ATTRIBUTE
 }
 
 void
 c_initialize_diagnostics (diagnostic_context *context)
 {
-  c_common_initialize_diagnostics (context);
-
   pretty_printer *base = context->printer;
   c_pretty_printer *pp = XNEW (c_pretty_printer);
   context->printer = new (pp) c_pretty_printer ();
 
   /* It is safe to free this object because it was previously XNEW()'d.  */
   base->~pretty_printer ();
   XDELETE (base);
+
+  c_common_diagnostics_defaults (context);
+  diagnostic_format_decoder (context) = &c_tree_printer;
 }
 
 int
 c_types_compatible_p (tree x, tree y)
 {
Index: gcc/cp/error.c
===================================================================
--- gcc/cp/error.c	(revision 215890)
+++ gcc/cp/error.c	(working copy)
@@ -37,15 +37,17 @@ along with GCC; see the file COPYING3.
 #include <new>                    // For placement-new.
 
 #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',')
 #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';')
 
-/* The global buffer where we dump everything.  It is there only for
-   transitional purpose.  It is expected, in the near future, to be
-   completely removed.  */
+/*  cxx_pp is a C++ front-end-specific pretty printer: this
+    is where we dump C++ ASTs as strings. It is mostly used only by
+    the various tree -> string functions that are occasionally
+    called from the debugger or by the front-end for things like
+    __PRETTY_FUNCTION__.  */
 static cxx_pretty_printer scratch_pretty_printer;
-#define cxx_pp (&scratch_pretty_printer)
+static cxx_pretty_printer * cxx_pp = &scratch_pretty_printer;
 
 /* Translate if being used for diagnostics, but not for dump files or
    __PRETTY_FUNCTION.  */
 #define M_(msgid) (pp_translate_identifiers (cxx_pp) ? _(msgid) : (msgid))
 
@@ -102,17 +104,38 @@ static void cp_diagnostic_starter (diagn
 static void cp_print_error_function (diagnostic_context *, diagnostic_info *);
 
 static bool cp_printer (pretty_printer *, text_info *, const char *,
 			int, bool, bool, bool);
 
+
+/* Construct a C++-aware pretty-printer for CONTEXT.  It is assumed
+   that CONTEXT->printer is an already constructed basic pretty_printer.  */
 void
-init_error (void)
+cxx_initialize_diagnostics (diagnostic_context *context)
 {
-  diagnostic_starter (global_dc) = cp_diagnostic_starter;
+  pretty_printer *base = context->printer;
+  cxx_pretty_printer *pp = XNEW (cxx_pretty_printer);
+  context->printer = new (pp) cxx_pretty_printer ();
+
+  /* It is safe to free this object because it was previously XNEW()'d.  */
+  base->~pretty_printer ();
+  XDELETE (base);
+
+  c_common_diagnostics_defaults (context);
+  diagnostic_starter (context) = cp_diagnostic_starter;
   /* diagnostic_finalizer is already c_diagnostic_finalizer.  */
-  diagnostic_format_decoder (global_dc) = cp_printer;
+  diagnostic_format_decoder (context) = cp_printer;
+}
 
+/*  cxx_pp is a C++ front-end-specific pretty printer: this
+    is where we dump C++ ASTs as strings. It is mostly used only by
+    the various tree -> string functions that are occasionally
+    called from the debugger or by the front-end for things like
+    __PRETTY_FUNCTION__.  */
+void
+init_error (void)
+{
   new (cxx_pp) cxx_pretty_printer ();
 }
 
 /* Dump a scope, if deemed necessary.  */
 
Index: gcc/cp/cxx-pretty-print.c
===================================================================
--- gcc/cp/cxx-pretty-print.c	(revision 215890)
+++ gcc/cp/cxx-pretty-print.c	(working copy)
@@ -2431,10 +2431,8 @@ typedef c_pretty_print_fn pp_fun;
 
 cxx_pretty_printer::cxx_pretty_printer ()
   : c_pretty_printer (),
     enclosing_scope (global_namespace)
 {
-  pp_set_line_maximum_length (this, 0);
-
   type_specifier_seq = (pp_fun) pp_cxx_type_specifier_seq;
   parameter_list = (pp_fun) pp_cxx_parameter_declaration_clause;
 }
Index: gcc/cp/cp-objcp-common.c
===================================================================
--- gcc/cp/cp-objcp-common.c	(revision 215890)
+++ gcc/cp/cp-objcp-common.c	(working copy)
@@ -30,12 +30,10 @@ along with GCC; see the file COPYING3.
 #include "diagnostic.h"
 #include "debug.h"
 #include "cxx-pretty-print.h"
 #include "cp-objcp-common.h"
 
-#include <new>                       // For placement new.
-
 /* Special routine to get the alias set for C++.  */
 
 alias_set_type
 cxx_get_alias_set (tree t)
 {
@@ -130,26 +128,10 @@ cp_var_mod_type_p (tree type, tree fn)
 
   /* All other types are not variably modified.  */
   return false;
 }
 
-/* Construct a C++-aware pretty-printer for CONTEXT.  It is assumed
-   that CONTEXT->printer is an already constructed basic pretty_printer.  */
-void
-cxx_initialize_diagnostics (diagnostic_context *context)
-{
-  c_common_initialize_diagnostics (context);
-
-  pretty_printer *base = context->printer;
-  cxx_pretty_printer *pp = XNEW (cxx_pretty_printer);
-  context->printer = new (pp) cxx_pretty_printer ();
-
-  /* It is safe to free this object because it was previously XNEW()'d.  */
-  base->~pretty_printer ();
-  XDELETE (base);
-}
-
 /* This compares two types for equivalence ("compatible" in C-based languages).
    This routine should only return 1 if it is sure.  It should not be used
    in contexts where erroneously returning 0 causes problems.  */
 
 int

Re: [PATCH diagnostics] PR 53061 cleanup initialization

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

> This is an old patch of mine that never got finished. I updated it following
> the suggestions of Gabriel here
> https://gcc.gnu.org/ml/gcc-patches/2012-04/msg00443.html

Thanks for looking at this again.

> Bootstrapped and tested on x86_64-linux-gnu.
>
> OK?

I think the patch is good.  I only have minor observations regarding
comments and one function naming.

> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi	(revision 215890)
> +++ gcc/doc/invoke.texi	(working copy)
> @@ -3075,15 +3075,14 @@ information should be reported.  Note th
>  honor these options.
>  
>  @table @gcctabopt
>  @item -fmessage-length=@var{n}
>  @opindex fmessage-length
> -Try to format error messages so that they fit on lines of about @var{n}
> -characters.  The default is 72 characters for @command{g++} and 0 for the rest of
> -the front ends supported by GCC@.  If @var{n} is zero, then no
> -line-wrapping is done; each error message appears on a single
> -line.
> +Try to format error messages so that they fit on lines of about
> +@var{n} characters.  If @var{n} is zero, then no line-wrapping will be
> +done; each error message will appear on a single line.  This is the
> +default for all front ends.

Agreed.

>  @item -fdiagnostics-show-location=once
>  @opindex fdiagnostics-show-location
>  Only meaningful in line-wrapping mode.  Instructs the diagnostic messages
>  reporter to emit source location information @emph{once}; that is, in
> Index: gcc/c-family/c-opts.c
> ===================================================================
> --- gcc/c-family/c-opts.c	(revision 215890)
> +++ gcc/c-family/c-opts.c	(working copy)
> @@ -176,25 +176,14 @@ c_diagnostic_finalizer (diagnostic_conte
>    virt_loc_aware_diagnostic_finalizer (context, diagnostic);
>    pp_destroy_prefix (context->printer);
>    pp_newline_and_flush (context->printer);
>  }
>  
> -/* Common diagnostics initialization.  */
> +/* Common default settings for diagnostics.  */
>  void
> -c_common_initialize_diagnostics (diagnostic_context *context)
> +c_common_diagnostics_defaults (diagnostic_context *context)

Please, call this c_common_diagnostics_set_defaults().

Having a verb in the function name (at least for functions that are not
accessors) makes it easier to read, I think.

>  {
> -  /* This is conditionalized only because that is the way the front
> -     ends used to do it.  Maybe this should be unconditional?  */
> -  if (c_dialect_cxx ())
> -    {
> -      /* By default wrap lines at 80 characters.  Is getenv
> -	 ("COLUMNS") preferable?  */
> -      diagnostic_line_cutoff (context) = 80;
> -      /* By default, emit location information once for every
> -	 diagnostic message.  */
> -      diagnostic_prefixing_rule (context) = DIAGNOSTICS_SHOW_PREFIX_ONCE;
> -    }
>    diagnostic_finalizer (context) = c_diagnostic_finalizer;
>    context->opt_permissive = OPT_fpermissive;
>  }

OK.

>  /* Whether options from all C-family languages should be accepted
> Index: gcc/c-family/c-common.h
> ===================================================================
> --- gcc/c-family/c-common.h	(revision 215890)
> +++ gcc/c-family/c-common.h	(working copy)
> @@ -824,11 +824,11 @@ extern void set_compound_literal_name (t
>  
>  extern tree build_va_arg (location_t, tree, tree);
>  
>  extern const unsigned int c_family_lang_mask;
>  extern unsigned int c_common_option_lang_mask (void);
> -extern void c_common_initialize_diagnostics (diagnostic_context *);
> +extern void c_common_diagnostics_defaults (diagnostic_context *);

c_common_diagnostics_defaults -> c_common_diagnostics_set_defaults.

>  extern bool c_common_complain_wrong_lang_p (const struct cl_option *);
>  extern void c_common_init_options_struct (struct gcc_options *);
>  extern void c_common_init_options (unsigned int, struct cl_decoded_option *);
>  extern bool c_common_post_options (const char **);
>  extern bool c_common_init (void);
> Index: gcc/c/c-objc-common.c
> ===================================================================
> --- gcc/c/c-objc-common.c	(revision 215890)
> +++ gcc/c/c-objc-common.c	(working copy)
> @@ -60,19 +60,11 @@ c_warn_unused_global_decl (const_tree de
>  bool
>  c_objc_common_init (void)
>  {
>    c_init_decl_processing ();
>  
> -  if (c_common_init () == false)
> -    return false;
> -
> -  /* These were not defined in the Objective-C front end, but I'm
> -     putting them here anyway.  The diagnostic format decoder might
> -     want an enhanced ObjC implementation.  */
> -  diagnostic_format_decoder (global_dc) = &c_tree_printer;
> -
> -  return true;
> +  return c_common_init ();
>  }

OK.

>  
>  /* Called during diagnostic message formatting process to print a
>     source-level entity onto BUFFER.  The meaning of the format specifiers
>     is as follows:
> @@ -184,19 +176,20 @@ has_c_linkage (const_tree decl ATTRIBUTE
>  }
>  
>  void
>  c_initialize_diagnostics (diagnostic_context *context)
>  {
> -  c_common_initialize_diagnostics (context);
> -
>    pretty_printer *base = context->printer;
>    c_pretty_printer *pp = XNEW (c_pretty_printer);
>    context->printer = new (pp) c_pretty_printer ();
>  
>    /* It is safe to free this object because it was previously XNEW()'d.  */
>    base->~pretty_printer ();
>    XDELETE (base);
> +
> +  c_common_diagnostics_defaults (context);

c_common_diagnostics_defaults -> c_common_diagnostics_set_defaults

> +  diagnostic_format_decoder (context) = &c_tree_printer;
>  }
>  
>  int
>  c_types_compatible_p (tree x, tree y)
>  {
> Index: gcc/cp/error.c
> ===================================================================
> --- gcc/cp/error.c	(revision 215890)
> +++ gcc/cp/error.c	(working copy)
> @@ -37,15 +37,17 @@ along with GCC; see the file COPYING3.
>  #include <new>                    // For placement-new.
>  
>  #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',')
>  #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';')
>  
> -/* The global buffer where we dump everything.  It is there only for
> -   transitional purpose.  It is expected, in the near future, to be
> -   completely removed.  */
> +/*  cxx_pp is a C++ front-end-specific pretty printer: this
> +    is where we dump C++ ASTs as strings. It is mostly used only by
> +    the various tree -> string functions that are occasionally
> +    called from the debugger or by the front-end for things like
> +    __PRETTY_FUNCTION__.  */

Thank you for the more extensive comment here.

>  static cxx_pretty_printer scratch_pretty_printer;
> -#define cxx_pp (&scratch_pretty_printer)
> +static cxx_pretty_printer * cxx_pp = &scratch_pretty_printer;

Yes!  I wonder why we were using a macro here in the first place.

>  /* Translate if being used for diagnostics, but not for dump files or
>     __PRETTY_FUNCTION.  */
>  #define M_(msgid) (pp_translate_identifiers (cxx_pp) ? _(msgid) : (msgid))
>  
> @@ -102,17 +104,38 @@ static void cp_diagnostic_starter (diagn
>  static void cp_print_error_function (diagnostic_context *, diagnostic_info *);
>  
>  static bool cp_printer (pretty_printer *, text_info *, const char *,
>  			int, bool, bool, bool);
>  
> +
> +/* Construct a C++-aware pretty-printer for CONTEXT.  It is assumed
> +   that CONTEXT->printer is an already constructed basic pretty_printer.  */

I'd be even more specific in the comment by saying that CONTEXT->printer
is a basic pretty printer that was constructed presumably by
diagnostic_initialize(), called early in the compiler's initialization
process (in general_init) Before the FE is initialized.  This (C++)
FE-specific diagnostic initializer is thus replacing the basic pretty
printer with one that has C++-aware capacities.

Or maybe write this generic big-picture awareness comment before the
diagnostic_context::printer data member.  If you don't have time for
this, I'll do it myself in a subsequent patch.  I am writing this, also
for myself, as a reminder :-)

>  void
> -init_error (void)
> +cxx_initialize_diagnostics (diagnostic_context *context)
>  {
> -  diagnostic_starter (global_dc) = cp_diagnostic_starter;
> +  pretty_printer *base = context->printer;
> +  cxx_pretty_printer *pp = XNEW (cxx_pretty_printer);
> +  context->printer = new (pp) cxx_pretty_printer ();
> +
> +  /* It is safe to free this object because it was previously XNEW()'d.  */
> +  base->~pretty_printer ();
> +  XDELETE (base);
> +
> +  c_common_diagnostics_defaults (context);
> +  diagnostic_starter (context) = cp_diagnostic_starter;
>    /* diagnostic_finalizer is already c_diagnostic_finalizer.  */
> -  diagnostic_format_decoder (global_dc) = cp_printer;
> +  diagnostic_format_decoder (context) = cp_printer;
> +}
>  
> +/*  cxx_pp is a C++ front-end-specific pretty printer: this
> +    is where we dump C++ ASTs as strings. It is mostly used only by
> +    the various tree -> string functions that are occasionally
> +    called from the debugger or by the front-end for things like
> +    __PRETTY_FUNCTION__.  */
> +void
> +init_error (void)

I think this comment should start saying that init_error initializes the
global cxx_pp variable that is used as the memory store for the string
representation of AST artifacts.  And then maybe refer to the comment of
cxx_pp for more details.

OK to commit with the above changes.

Thanks.

-- 
		Dodji

Re: [PATCH diagnostics] PR 53061 cleanup initialization

[Manuel López-Ibáñez]

I committed this as https://gcc.gnu.org/r216720 following all your
comments except for:


On 23 October 2014 12:31, Dodji Seketeli <dodji@redhat.com> wrote:
>> +
>> +/* Construct a C++-aware pretty-printer for CONTEXT.  It is assumed
>> +   that CONTEXT->printer is an already constructed basic pretty_printer.  */
>
> I'd be even more specific in the comment by saying that CONTEXT->printer
> is a basic pretty printer that was constructed presumably by
> diagnostic_initialize(), called early in the compiler's initialization
> process (in general_init) Before the FE is initialized.  This (C++)
> FE-specific diagnostic initializer is thus replacing the basic pretty
> printer with one that has C++-aware capacities.
>
> Or maybe write this generic big-picture awareness comment before the
> diagnostic_context::printer data member.  If you don't have time for
> this, I'll do it myself in a subsequent patch.  I am writing this, also
> for myself, as a reminder :-)

I did the former and not the latter because the basic pp does not need
to be overridden, the FEs can do it but they don't need to. Thus, I
was not sure what you really wanted me to write in diagnostic.h The
current comment is:

/* Where most of the diagnostic formatting work is done.  */
  pretty_printer *printer;

which admittedly is not that informative.

Cheers,

Manuel.