This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH diagnostics] PR 53061 cleanup initialization
- From: Dodji Seketeli <dodji at redhat dot com>
- To: Manuel LÃpez-IbÃÃez <lopezibanez at gmail dot com>
- Cc: Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 23 Oct 2014 12:31:42 +0200
- Subject: Re: [PATCH diagnostics] PR 53061 cleanup initialization
- Authentication-results: sourceware.org; auth=none
- References: <CAESRpQCJRGpewUNm8GJx8hr0wqLx7RUVKh5OUkL=cGw-ETe3sg at mail dot gmail dot com>
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