Re: [PATCH 04/10] Reimplement diagnostic_show_locus, introducing rich_location classes (v5)

[Jeff Law <law@redhat.com>]

On 10/23/2015 02:41 PM, David Malcolm wrote:
> The change since v4 can be seen at:
>   https://dmalcolm.fedorapeople.org/gcc/2015-10-23/0001-Add-colorize_source_p-to-diagnostic_context.patch
> which is a tweak to colorization, to handle both frontends that provide
> ranges and those that only provide carets, and provide a smoother
> transition path for the latter.
>
> gcc/ChangeLog:
> 	* diagnostic-color.c (color_dict): Eliminate "caret"; add "range1"
> 	and "range2".
> 	(parse_gcc_colors): Update comment to describe default GCC_COLORS.
> 	* diagnostic-core.h (warning_at_rich_loc): New declaration.
> 	(error_at_rich_loc): New declaration.
> 	(permerror_at_rich_loc): New declaration.
> 	(inform_at_rich_loc): New declaration.
> 	* diagnostic-show-locus.c (adjust_line): Delete.
> 	(struct point_state): New struct.
> 	(class colorizer): New class.
> 	(class layout_point): New class.
> 	(class layout_range): New class.
> 	(class layout): New class.
> 	(colorizer::colorizer): New ctor.
> 	(colorizer::~colorizer): New dtor.
> 	(layout::layout): New ctor.
> 	(layout::print_line): New method.
> 	(layout::get_state_at_point): New method.
> 	(layout::get_x_bound_for_row): New method.
> 	(show_ruler): New function.
> 	(diagnostic_show_locus): Reimplement in terms of class layout.
> 	* diagnostic.c (diagnostic_initialize): Replace
> 	MAX_LOCATIONS_PER_MESSAGE with rich_location::MAX_RANGES.
> 	(diagnostic_set_info_translated): Convert param from location_t
> 	to rich_location *.  Eliminate calls to set_location on the
> 	message in favor of storing the rich_location ptr there.
> 	(diagnostic_set_info): Convert param from location_t to
> 	rich_location *.
> 	(diagnostic_build_prefix): Break out array into...
> 	(diagnostic_kind_color): New variable.
> 	(diagnostic_get_color_for_kind): New function.
> 	(diagnostic_report_diagnostic): Colorize the option_text
> 	using the color for the severity.
> 	(diagnostic_append_note): Update for change in signature of
> 	diagnostic_set_info.
> 	(diagnostic_append_note_at_rich_loc): New function.
> 	(emit_diagnostic): Update for change in signature of
> 	diagnostic_set_info.
> 	(inform): Likewise.
> 	(inform_at_rich_loc): New function.
> 	(inform_n): Update for change in signature of diagnostic_set_info.
> 	(warning): Likewise.
> 	(warning_at): Likewise.
> 	(warning_at_rich_loc): New function.
> 	(warning_n): Update for change in signature of diagnostic_set_info.
> 	(pedwarn): Likewise.
> 	(permerror): Likewise.
> 	(permerror_at_rich_loc): New function.
> 	(error): Update for change in signature of diagnostic_set_info.
> 	(error_n): Likewise.
> 	(error_at): Likewise.
> 	(error_at_rich_loc): New function.
> 	(sorry): Update for change in signature of diagnostic_set_info.
> 	(fatal_error): Likewise.
> 	(internal_error): Likewise.
> 	(internal_error_no_backtrace): Likewise.
> 	(source_range::debug): New function.
> 	* diagnostic.h (struct diagnostic_info): Eliminate field
> 	"override_column".  Add field "richloc".
> 	(struct diagnostic_context): Add field "colorize_source_p".
> 	(diagnostic_override_column): Delete.
> 	(diagnostic_set_info): Convert param from location_t to
> 	rich_location *.
> 	(diagnostic_set_info_translated): Likewise.
> 	(diagnostic_append_note_at_rich_loc): New function.
> 	(diagnostic_num_locations): New function.
> 	(diagnostic_expand_location): Get the location from the
> 	rich_location.
> 	(diagnostic_print_caret_line): Delete.
> 	(diagnostic_get_color_for_kind): New declaration.
> 	* genmatch.c (linemap_client_expand_location_to_spelling_point): New.
> 	(error_cb): Update for change in signature of "error" callback.
> 	(fatal_at): Likewise.
> 	(warning_at): Likewise.
> 	* input.c (linemap_client_expand_location_to_spelling_point): New.
> 	* pretty-print.c (text_info::set_range): New method.
> 	(text_info::get_location): New method.
> 	* pretty-print.h (MAX_LOCATIONS_PER_MESSAGE): Eliminate this macro.
> 	(struct text_info): Eliminate "locations" array in favor of
> 	"m_richloc", a rich_location *.
> 	(textinfo::set_location): Add a "caret_p" param, and reimplement
> 	in terms of a call to set_range.
> 	(textinfo::get_location): Eliminate inline implementation in favor of
> 	an out-of-line reimplementation.
> 	(textinfo::set_range): New method.
> 	* rtl-error.c (diagnostic_for_asm): Update for change in signature
> 	of diagnostic_set_info.
> 	* tree-diagnostic.c (default_tree_printer): Update for new
> 	"caret_p" param for textinfo::set_location.
> 	* tree-pretty-print.c (percent_K_format): Likewise.
>
> gcc/c-family/ChangeLog:
> 	* c-common.c (c_cpp_error): Convert parameter from location_t to
> 	rich_location *.  Eliminate the "column_override" parameter and
> 	the call to diagnostic_override_column.
> 	Update the "done_lexing" clause to set range 0
> 	on the rich_location, rather than overwriting a location_t.
> 	* c-common.h (c_cpp_error): Convert parameter from location_t to
> 	rich_location *.  Eliminate the "column_override" parameter.
>
> gcc/c/ChangeLog:
> 	* c-decl.c (warn_defaults_to): Update for change in signature
> 	of diagnostic_set_info.
> 	* c-errors.c (pedwarn_c99): Likewise.
> 	(pedwarn_c90): Likewise.
> 	* c-objc-common.c (c_tree_printer): Update for new "caret_p" param
> 	for textinfo::set_location.
>
> gcc/cp/ChangeLog:
> 	* error.c (cp_printer): Update for new "caret_p" param for
> 	textinfo::set_location.
> 	(pedwarn_cxx98): Update for change in signature of
> 	diagnostic_set_info.
>
> gcc/fortran/ChangeLog:
> 	* cpp.c (cb_cpp_error): Convert parameter from location_t to
> 	rich_location *.  Eliminate the "column_override" parameter.
> 	* error.c (gfc_warning): Update for change in signature of
> 	diagnostic_set_info.
> 	(gfc_format_decoder): Update handling of %C/%L for changes
> 	to struct text_info.
> 	(gfc_diagnostic_starter): Use richloc when determining whether to
> 	print one locus or two.  When handling a location that will
> 	involve a call to diagnostic_show_locus, only attempt to print the
> 	locus for the primary location, and don't call into
> 	diagnostic_print_caret_line.
> 	(gfc_warning_now_at): Update for change in signature of
> 	diagnostic_set_info.
> 	(gfc_warning_now): Likewise.
> 	(gfc_error_now): Likewise.
> 	(gfc_fatal_error): Likewise.
> 	(gfc_error): Likewise.
> 	(gfc_internal_error): Likewise.
>
> gcc/testsuite/ChangeLog:
> 	* gcc.dg/plugin/diagnostic-test-show-locus-bw.c: New file.
> 	* gcc.dg/plugin/diagnostic-test-show-locus-color.c: New file.
> 	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c: New file.
> 	* gcc.dg/plugin/plugin.exp (plugin_test_list): Add the above.
> 	* lib/gcc-dg.exp: Load multiline.exp.
>
> libcpp/ChangeLog:
> 	* errors.c (cpp_diagnostic): Update for change in signature
> 	of "error" callback.
> 	(cpp_diagnostic_with_line): Likewise, calling override_column
> 	on the rich_location.
> 	* include/cpplib.h (struct cpp_callbacks): Within "error"
> 	callback, convert param from source_location to rich_location *,
> 	and drop column_override param.
> 	* include/line-map.h (struct source_range): New struct.
> 	(struct location_range): New struct.
> 	(class rich_location): New class.
> 	(linemap_client_expand_location_to_spelling_point): New declaration.
> 	* line-map.c (rich_location::rich_location): New ctors.
> 	(rich_location::lazily_expand_location): New method.
> 	(rich_location::override_column): New method.
> 	(rich_location::add_range): New methods.
> 	(rich_location::set_range): New method.
> ---

> diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
> index 147a2b8..6865209 100644
> --- a/gcc/diagnostic-show-locus.c
> +++ b/gcc/diagnostic-show-locus.c
I'm having trouble breaking this down into manageable hunks to look at. 
  Are there bits in here that we can pull out as separate patches?  It 
looks like git diff is just making a mess of this file when I think it's 
a huge chunk of new code and a few deletes.

If you have a blob of new code and a blob of deletes, even breaking it 
down that way may help in this case (ie, a patch with new classes & 
code, then a pass that deletes old crud we're not going to use anymore).






> +
> +void
> +source_range::debug (const char *msg) const
> +{
> +  rich_location richloc (m_start);
> +  richloc.add_range (m_start, m_finish);
> +  inform_at_rich_loc (&richloc, "%s", msg);
> +}
Function comment.  Do you need a DEBUG_FUNCTION annotation here?


> +extern const char *
> +diagnostic_get_color_for_kind (diagnostic_t kind);
If this will fit on one line, then combine the lines.


>
>   /* Pure text formatting support functions.  */
>   extern char *file_name_as_prefix (diagnostic_context *, const char *);

> diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
> index 3825751..4b3d31c 100644
> --- a/gcc/fortran/error.c
> +++ b/gcc/fortran/error.c
I'm having a hard time mapping the code you removed from 
fortran/error.c::gfc_diagnostic_starter to its functional equivalent in 
your new code.  I know we've discussed this issue a few times on the 
phone, so I don't doubt you're handling it.  I just want to know where 
so I can double-check things a bit.




> diff --git a/gcc/genmatch.c b/gcc/genmatch.c
> index 102a635..6bfde06 100644
> --- a/gcc/genmatch.c
> +++ b/gcc/genmatch.c
> @@ -53,14 +53,23 @@ unsigned verbose;
>
>   static struct line_maps *line_table;
>
> +expanded_location
> +linemap_client_expand_location_to_spelling_point (source_location loc)
> +{
> +  const struct line_map_ordinary *map;
> +  loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION, &map);
> +  return linemap_expand_location (line_table, map, loc);
> +}
Function comment.


> diff --git a/gcc/input.c b/gcc/input.c
> index ff80dd9..baf8e7e 100644
> --- a/gcc/input.c
> +++ b/gcc/input.c
> @@ -751,6 +751,13 @@ expand_location_to_spelling_point (source_location loc)
>     return expand_location_1 (loc, /*expansion_point_p=*/false);
>   }
>
> +expanded_location
> +linemap_client_expand_location_to_spelling_point (source_location loc)
> +{
> +  return expand_location_to_spelling_point (loc);
> +}
Likewise.


> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index 09378f9..84a5ab7 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -131,6 +131,35 @@ typedef unsigned int linenum_type;
>     libcpp/location-example.txt.  */
>   typedef unsigned int source_location;
>
> +/* A range of source locations.
> +
> +   Ranges are closed:
> +   m_start is the first location within the range,
> +   m_finish is the last location within the range.
> +
> +   We may need a more compact way to store these, but for now,
> +   let's do it the simple way, as a pair.  */
> +struct GTY(()) source_range
> +{
> +  source_location m_start;
> +  source_location m_finish;
> +
> +  void debug (const char *msg) const;
Do you need a DEBUG_FUNCTION annotation here?

> @@ -1028,6 +1057,175 @@ typedef struct
>     bool sysp;
>   } expanded_location;
>
> +class rich_location
[ ... ]

> +
> +  void
> +  add_range (source_location start, source_location finish,
> +	     bool show_caret_p = false);
> +
> +  void
> +  add_range (source_range src_range,
> +	     bool show_caret_p = false);
Do we really want to bother with default arguments?  Is it buying us 
some level of cleanliness that's hard to otherwise achieve?  Given this 
is new code I just don't see the value here.  Educate me.

Jeff