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

[David Malcolm <dmalcolm at redhat dot com>]

On Fri, 2015-09-25 at 23:13 +0200, Manuel LÃpez-IbÃÃez wrote:
> +   If SHOW_CARET_P is true, then the range should be rendered with
> +   a caret at its starting location.  This
> +   is for use by the Fortran frontend, for implementing the
> +   "%C" and "%L" format codes.  */
> +
> +void
> +rich_location::set_range (unsigned int idx, source_range src_range,
> +              bool show_caret_p, bool overwrite_loc_p)
> 
> I do not understand when is this show_caret_p used by Fortran given
> the diagnostic_show_locus code mentioned earlier.

The patch is something of a hybrid: on the one hand it's using the new
rich_location class for storing multiple locations for a diagnostic (and
this replaces the existing way we did this in struct text_info), but on
the other hand, for Fortran, it's using the old printing code.

rich_location::set_range exists to ensure that the %C and %L codes used
by Fortran (and "+" in the C family of FEs) can write back into the
rich_location instance, faithfully emulating the old code that wrote
back to
struct text_info's:
  location_t locations[MAX_LOCATIONS_PER_MESSAGE];

(that array is replaced in the patch by a rich_location *, pointing back
at the rich_location in the diagnostic_info).

> Related to this:
> 
> inline void set_location (unsigned int idx, location_t loc, bool caret_p)
> 
> is always called with the last parameter 'true' (boolean parameters
> are always almost bad API). Do you really need this parameter?

Ah, OK.  Maybe not there.

> +/* Overwrite the range within this text_info's rich_location.
> +   For use e.g. when implementing "+" in client format decoders.  */
> 
> If we got rid of '+' we would not need this extra work. Also '+'
> breaks #pragma diagnostics. Not the fault of your patch, but it just
> shows that technical debt keeps accumulating.
> https://gcc.gnu.org/wiki/Partial_Transitions

(nods)   That "+" thing was one of the surprises I ran into when working
on this, and is the reason that it isn't a :
  const rich_location *
but just a:
  rich_location *
given that formatting the diagnostic text can lead to the location being
modified.  I'm just emulating/supporting the existing behavior.

Dave