Re: [PATCH diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls

[Dodji Seketeli <dodji@redhat.com>]

Hello Manuel,

Sorry for my late reply, and thank you very much for working on this.

I have looked at the patch and I like it!

I guess I just have some few lateral nits to pick.

> The Fortran FE allows diagnostics with two different locations.
> Depending on whether these locations are on the same line or not, this
> may produce one or two caret lines. This is the last remaining issue
> left to make Fortran diagnostics use the common code.

Yes, I remember.

[...]

> I added support for this in the common diagnostics, although Fortran
> is the only user for now. The new common code should be flexible
> enough to support the Clang style (which I guess is likely to be what
> C/C++ FEs end up supporting sooner or later) while still supporting
> the Fortran style.

Good.

> Supporting this in the common diagnostics code requires having two
> locations in struct diagnostic_info and two pointers in struct
> text_info. That seems a waste and overtly complex. Thus, I moved the
> new location array directly to struct text_info and added an accessor
> function diagnostic_location().

Agreed.

[...]


> Index: gcc/pretty-print.h

[...]

> --- gcc/pretty-print.h	(revision 222087)
> +++ gcc/pretty-print.h	(working copy)
> @@ -33,11 +33,13 @@ along with GCC; see the file COPYING3.  
>  struct text_info
>  {
>    const char *format_spec;
>    va_list *args_ptr;
>    int err_no;  /* for %m */
> -  location_t *locus;
> +  /* This message can have associated two locations at most.  If the
> +     first location is UNKNOWN_LOCATION, the second is not valid.  */
> +  location_t location[2];

Here, I would call the data member locations (note the 's' at the
end).

Also, I'd define a constant (a macro, sigh) named like e.g,
MAX_LOCATIONS_PER_MESSAGE that is set to '2', rather than carrying
forcing users of these locations to know that there are specifically
two locations here.

[...]

>    void **x_data;
>  };
>  


> Index: gcc/diagnostic.h

[..]

>  /* A diagnostic is described by the MESSAGE to send, the FILE and LINE of
>     its context and its KIND (ice, error, warning, note, ...)  See complete
>     list in diagnostic.def.  */
>  struct diagnostic_info
>  {
> +  /* Text to be formatted. It also contains the location(s) for this
> +     diagnostic.  */
>    text_info message;
> -  location_t location;
>    unsigned int override_column;

[...]

>  
> -  /* Character used for caret diagnostics.  */
> -  char caret_char;
> +  /* Characters used for caret diagnostics.  */
> +  char caret_char[2];
>  

Here, I'd call the data member caret_chars (with an 's' at the end)
and I'd use the same MAX_LOCATIONS_PER_MESSAGE constant as above,
rather that the '2' literal.

[...]

> --- gcc/tree-pretty-print.c	(revision 222087)
> +++ gcc/tree-pretty-print.c	(working copy)

[...]

> @@ -3618,12 +3618,11 @@ newline_and_indent (pretty_printer *pp, 
>  
>  void
>  percent_K_format (text_info *text)
>  {
>    tree t = va_arg (*text->args_ptr, tree), block;
> -  gcc_assert (text->locus != NULL);
> -  *text->locus = EXPR_LOCATION (t);
> +  text->location[0] = EXPR_LOCATION (t);

I guess I'd prefer to have an accessor (e.g,
source_location text_info_location(text_info, int index_of_location))
that returns the location and checks that we are accessing a location
that is below the MAX_LOCATIONS_PER_MESSAGE maximum, rather than just
doing text->locations[0] here.

And, likewise for the other similar spots that access
text_info::locations.

[...]

> --- gcc/diagnostic.c	(revision 222087)
> +++ gcc/diagnostic.c	(working copy)
> @@ -144,11 +144,12 @@ diagnostic_initialize (diagnostic_contex

[...]

>    context->show_caret = false;
>    diagnostic_set_caret_max_width (context, pp_line_cutoff (context->printer));
> -  context->caret_char = '^';
> +  context->caret_char[0] = '^';
> +  context->caret_char[1] = '^';

I'd use a loop from O to MAX_LOCATIONS_PER_MESSAGE to initialize
this.  Or maybe rather a dedicated little function for this even; as
you see fit.

[...]

> @@ -239,11 +240,12 @@ diagnostic_set_info_translated (diagnost
>  				diagnostic_t kind)
>  {
>    diagnostic->message.err_no = errno;
>    diagnostic->message.args_ptr = args;
>    diagnostic->message.format_spec = msg;
> -  diagnostic->location = location;
> +  diagnostic->message.location[0] = location;
> +  diagnostic->message.location[1] = UNKNOWN_LOCATION;

I'd use a loop from 1 to to MAX_LOCATIONS_PER_MESSAGE to set the
UNKNOWN_LOCATION.  I understand that the loop will step only one
iteration, but the goal is to be ready for when a front end is going
to need three or more locations per messages.  We'd then just need to
to adjust MAX_LOCATIONS_PER_MESSAGE and the whole thing would "almost"
Just Work ™.

[...]

>  /* Print the physical source line corresponding to the location of
> -   this diagnostic, and a caret indicating the precise column.  */
> +   this diagnostic, and a caret indicating the precise column.  This
> +   function only prints two caret characters if the two locations given by
> +   DIAGNOSTIC are on the same locus according to

I am confused by what you mean by "same locus" here.  Do you mean the
"same line" ?  If yes, then maybe the other spots where you talk about
"same locus" should be updated too.  More on this later below.

> +   diagnostic_same_locus().  */
>  void
>  diagnostic_show_locus (diagnostic_context * context,
>  		       const diagnostic_info *diagnostic)
>  {
> -  const char *line;
> -  int line_width;
> -  char *buffer;
> -  expanded_location s;
> -  int max_width;
> -  const char *saved_prefix;
> -  const char *caret_cs, *caret_ce;
> +  expanded_location s1, s2;

Here, I'd initialize s1 and s2.  Well, at least s2.  So that ...

>    if (!context->show_caret
> -      || diagnostic->location <= BUILTINS_LOCATION
> -      || diagnostic->location == context->last_location)
> +      || diagnostic_location (diagnostic, 0) <= BUILTINS_LOCATION
> +      || diagnostic_location (diagnostic, 0) == context->last_location)
>      return;
>  
> -  context->last_location = diagnostic->location;
> -  s = diagnostic_expand_location (diagnostic);
> -  line = location_get_source_line (s, &line_width);
> -  if (line == NULL || s.column > line_width)
> +  context->last_location = diagnostic_location (diagnostic, 0);
> +  s1 = diagnostic_expand_location (diagnostic, 0);
> +  if (diagnostic_location (diagnostic, 1) > BUILTINS_LOCATION)
> +    s2 = diagnostic_expand_location (diagnostic, 1);
> +  else
> +    s2.column = 0;

... we can do away with the else branch here.

[...]

I would add a comment to this new function here:

> -/* Expand the location of this diagnostic. Use this function for consistency. */
> +static inline location_t
> +diagnostic_location (const diagnostic_info * diagnostic, int which = 0)
> +{
> +  return diagnostic->message.location[which];
> +}
> +
> +
> +/* Expand the location of this diagnostic. Use this function for consistency.
> +   WHICH specifies which location. By default, expand the first one.  */

[...]

>  
> +/* Return true if the two locations can be represented within the same
> +   locus.  This is used not only for the prefix but also to determine
> +   whether to print one or two caret lines.  */
> +
> +static inline bool
> +diagnostic_same_locus (const diagnostic_context *context,
> +		       expanded_location s1, expanded_location s2)
> +{
> +  return s2.column && s1.line == s2.line 
> +    && context->caret_max_width - 10 > abs (s1.column - s2.column);
> +}

My understanding is that this function checks that the two locations s1
and s2 are on the same line and are "close enough"; that is, they fit
into the maximum "display width" that the diagnostic machinery allows
for a line.  That display width is, from I see in adjust_line(),
diagnostic_context::caret_max_width minus a margin; and the margin is
10.

If I am right, then I think the name diagnostic_same_locus() should be
changed to something more meaningful.  The 'locus' here seems
confusing and makes the code hard to understand, IMHO.  Especially
given that we are talking about 'locations' as well.  What do you
think?

I would also change the 10 literal into a named constant and use it at
the other spots where we use the 10 today.  Because otherwise, seeing
that 10 literal magically appear in this function like this is
... surprising.  Is it not?

[...]

> Bootstrapped and regression tested on x86_64-linux-gnu.

Thanks!

> 
> OK?
> 

It's mostly OK to me, barring the points I have raised and for which I
need input from you.  Sorry again for taking so much time in reviewing
this.

Thanks!

-- 
		Dodji