v3 of diagnostic_show_locus and rich_location (was Re: [PATCH 2/5] Reimplement diagnostic_show_locus, introducing rich_location classes (v2))

On Fri, 2015-09-25 at 10:51 +0200, Dodji Seketeli wrote:
> Hello David,
> 
> I like this!  Thank you very much for working on this.

Thanks for the review.

> I think this patch is in great shape, and once we agree on some of the
> nits I have commented on below, I think it should go in. Oh, it also
> needs the first patch (1/5, dejagnu first) to go in first, as this one
> depends on it.  I defer to the dejagnu maintainers for that one.

Indeed.  Jeff just approved it, fwiw.

> The line-map parts are OK to me too, but I have no power on those.  So I
> defer to the FE maintainers for that.  The diagnostics parts of the
> Fortran, C++ and C FE look good to me too; these are just well contained
> mechanical adjustments, but I defer to FE maintainers for the final
> word.
> 
> Please find my comments below.

Updated patch attached.

> [...]
> 
> diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
> 
> [...]
> 
> +/* A class to inject colorization codes when printing the diagnostic locus,
> +   tracking state as it goes.  */
> +
> +class colorizer
> +{
> 
> [...]
> 
>     +  void set_state (int state);
>     +  void begin_state (int state);
>     +  void finish_state (int state);
> 
> I think the concept of state could use a little bit of explanation, at
> least to say that there are the same number of states, as the number
> of ranges.  And that the 'state' argument to these functions really is
> the range index.

Here's the revised comment I put in the attached patch:

+/* A class to inject colorization codes when printing the diagnostic locus.
+
+   It has one kind of colorization for each of:
+     - normal text
+     - range 0 (the "primary location")
+     - range 1
+     - range 2
+
+   The class caches the lookup of the color codes for the above.
+
+   The class also has responsibility for tracking which of the above is
+   active, filtering out unnecessary changes.  This allows layout::print_line
+   to simply request a colorization code for *every* character it prints
+   thorough this class, and have the filtering be done for it here.  */

Hopefully that comment explains the possible states the colorizer can
have.

FWIW I have a follow-up patch to add support for fix-it hints, so they
might be another kind of colorization state.
(see https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00732.html for the
earlier version of said patch, in v1 of the kit).

> Also, I am thinking that there should maybe be a layout::state type,
> which would have two notional properties (for now): range_index and
> draw_caret_p. So that this function:
> 
> +bool
> +layout::get_state_at_point (/* Inputs.  */
> +			    int row, int column,
> +			    int first_non_ws, int last_non_ws,
> +			    /* Outputs.  */
> +			    int *out_range_idx,
> +			    bool *out_draw_caret_p)
> 
> Would take just one output parameter, e.g, a reference to
> layout::state.

Fixed, though I called it "struct point_state", given that it's coming
from get_state_at_point.  I passed it by pointer, since AFAIK our coding
standards don't yet approve of the use of references in the codebase
(outside of places where we need them e.g. container classes).

I also added a unit test for a rich_location with two caret locations
(mimicking one of the Fortran examples), to give us coverage for this
case:

+void test_multiple_carets (void)
+{
+#if 0
+   x = x + y /* { dg-warning "8: test" } */
+/* { dg-begin-multiline-output "" }
+    x = x + y
+        A   B
+   { dg-end-multiline-output "" } */
+#endif
+}

where the "A" and "B" as caret chars are coming from new code in the
show_locus unittest plugin.

> +layout::layout (diagnostic_context * context,
> +		const diagnostic_info *diagnostic)
> 
> [...]
> 
> +      if (loc_range->m_finish.file != m_exploc.file)
> +	continue;
> +      if (loc_range->m_show_caret_p)
> +	if (loc_range->m_finish.file != m_exploc.file)
> +	  continue;
> 
> I think the second if clause is redundant.

Good catch; thanks.  The second if clause was meant to be testing
m_caret.file.  Fixed in the updated patch.

> 
> +  if (0)
> +    show_ruler (context, line_width, m_x_offset);
> 
> This should probably be removed from the final code to be committed.

FWIW, the ruler is very helpful to me when debugging the locus-printing
(e.g. when adding fix-it-hints), and if we remove that if (0) call, we
get:

warning: ‘void show_ruler(diagnostic_context*, int, int)’ defined but
not used [-Wunused-function]

which will break bootstrap, so perhaps it instead should be an option?
"-fdiagnostics-show-ruler" or somesuch?

I don't know that it would be helpful to end-users though.

I'd prefer to just keep it in the code with the
  if (0)
as-is, since it's useful "scaffolding" for hacking on the code.

> [...]
> 
> +/* Get the column beyond the rightmost one that could contain a caret or
> +   range marker, given that we stop rendering at trailing whitespace.  */
> +
> +int
> +layout::get_x_bound_for_row (int row, int caret_column,
> +			     int last_non_ws)
> 
> Please describe what the parameters mean here, especially last_non_ws.
> I had to read its code to know that last_non_ws was the *column* of
> the last non white space character.

I renamed it to "last_non_ws_column", and fleshed out the comment:

-/* Get the column beyond the rightmost one that could contain a caret or
-   range marker, given that we stop rendering at trailing whitespace.  */
+/* Helper function for use by layout::print_line when printing the
+   annotation line under the source line.
+   Get the column beyond the rightmost one that could contain a caret or
+   range marker, given that we stop rendering at trailing whitespace.
+   ROW is the source line within the given file.
+   CARET_COLUMN is the column of range 0's caret.
+   LAST_NON_WS_COLUMN is the last column containing a non-whitespace
+   character of source (as determined when printing the source line).  */

> [...]
> 
> +void
> +layout::print_line (int row)
> 
> This function is neat.  I like it! :-)

:)

> [...]
> 
>  void
>  diagnostic_show_locus (diagnostic_context * context,
>  		       const diagnostic_info *diagnostic)
> @@ -75,16 +710,25 @@ diagnostic_show_locus (diagnostic_context * context,
>      return;
> 
> +      /* The GCC 5 routine. */
> 
> I'd say the GCC <= 5 routine ;-)

> +  else
> +    /* The GCC 6 routine.  */
> 
> And here, the GCC > 5 routine.

Changed to "GCC < 6" and "GCC >= 6", on the pedantic grounds that e.g.
5.1 > 5

> I would be surprised to see this patch in particular incur any
> noticeable increase in time and space consumption, but, have you noticed
> anythying related to that during bootstrap?

I hadn't noticed it, but I wasn't timing.  I'll have a look.

One possible nit here is that the patch expands locations when
constructing rich_location instances, and it does that for warnings
before the logic to ignore them.  So there may be some extra calls there
that aren't present in trunk, for discarded warnings.  I don't expect
that to affect the speed of the compiler though (I expect it to be lost
in the noise).

Updated patch attached.  It compiles; a bootstrap/regrtest is in
progress, but may not be done before I disappear on vacation.  I believe
it addresses all of the points you raised apart from the show_ruler one.

OK for trunk if it passes bootstrap/regrtest?
(see https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01700.html for the
supporting blurb for v2).

Dave

v3 of diagnostic_show_locus and rich_location (was Re: [PATCH 2/5] Reimplement diagnostic_show_locus, introducing rich_location classes (v2))

Commit Message

Comments

Patch