On Tue, 2015-10-27 at 17:02 -0600, Jeff Law wrote: > 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).
I've split patch 4 of the kit into 3 sub-patches: [PATCH 4a] diagnostic-show-locus.c changes: Deletions [PATCH 4b] diagnostic-show-locus.c changes: Insertions [PATCH 4c] Other changes: everything apart from diagnostic-show-locus.c changes 4a, 4b, and 4c should appear as followups to this mail (assuming my "git send-email" command works OK). They're only split up for ease-of-review purposes; they're intended to be committed as one commit. The 4a/4b split seems to have allowed "git diff" to done a much better job on diagnostic-show-locus.c. Patch 4c contains updates based on your review comments below; a diff relative to the prior version of the patch can be seen at: https://dmalcolm.fedorapeople.org/gcc/2015-10-28/rich-locations/0001-Fix-issues-found-in-review.patch > > +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? Added function comment and DEBUG_FUNCTION annotation. Note that this is slightly messy: the method is declared in libcpp (in libcpp/include/line-map.h), but defined in gcc: it uses the gcc diagnostics-printing machinery to print a note highlighting the range. The method is sufficently useful for debugging to warrant this layering violation, in my opionion (I've been using it all the time when working on this code). I've called out this wart in the new comments. > > +extern const char * > > +diagnostic_get_color_for_kind (diagnostic_t kind); > If this will fit on one line, then combine the lines. Fixed. > > /* 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. Both old and new implementations of gfc_diagnostic_starter determine if we have just one locus, or more than one (bool one_locus). Both old and new implementations determine if diagnostic_show_locus is going to print anything. There's some logic for the no-caret-printed case to print multiple prefix lines, which is the same for both old and new implementations; this is what the existing test cases look for (the existing test cases all implicitly use -fno-diagnostics-show-caret and so use this branch). For the caret-printing case, both old and new implementation call diagnostic_show_locus. The old implementation would then call some follow-up code, which is what the removed code is: it would determine if the 2nd caret was on a different line, and if so, print that line via diagnostic_print_caret_line. This followup code was needed by the old implementation since diagnostic_show_locus would only print a single line of source code (the one with the caret). It's not needed by the new implementation since diagnostic_show_locus will print the lines containing both carets (and any lines in between). > > 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. Fixed. > > 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. Fixed. > > 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? As noted above, this method is declared in libcpp but defined in gcc. DEBUG_FUNCTION is in gcc, so is unavailable in libcpp. I've added a function comment here, which notes this. > > @@ -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. I think I was just being lazy. Almost all calls were with show_caret_p == false, but those were in the test plugin. I've eliminated the default values for the arguments, supplying explicit values. Hopefully this addresses your concerns, and the -show-locus.c code is now reviewable. Is the combined 4a+4b+4c patch OK for trunk, assuming it passes bootstrap®test? (running it now) Thanks Dave
