| Message ID | CAESRpQCJRGpewUNm8GJx8hr0wqLx7RUVKh5OUkL=cGw-ETe3sg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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.