Hello Manuel, Manuel López-Ibáñez <lopeziba...@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
