[PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Dodji Seketeli]

Hello Manuel,

Manuel López-Ibáñez <lopezibanez@gmail.com> writes:


> Dodji, are the linemap_asserts() appropriate?

Yes they are.  I have some additional comments though.

> libcpp/ChangeLog:
>
> 2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>
>     PR fortran/44054
>     * include/line-map.h (linemap_position_for_loc_and_offset):
>     Declare.
>     * line-map.c (linemap_position_for_loc_and_offset): New.
[...]

> --- libcpp/include/line-map.h	(revision 216257)
> +++ libcpp/include/line-map.h	(working copy)
> @@ -601,10 +601,17 @@ linemap_position_for_column (struct line
>     column.  */
>  source_location
>  linemap_position_for_line_and_column (const struct line_map *,
>  				      linenum_type, unsigned int);
>  
> +/* Encode and return a source_location starting from location LOC
> +   and shifting it by OFFSET columns.  */
> +source_location
> +linemap_position_for_loc_and_offset (struct line_maps *set,
> +				     source_location loc,
> +				     unsigned int offset);
> +

OK.

[...]

> --- libcpp/line-map.c	(revision 216257)
> +++ libcpp/line-map.c	(working copy)

[...]

> +/* Encode and return a source_location starting from location LOC
> +   and shifting it by OFFSET columns.  */
> +

The comment is OK.  I would just add that this function currently only
works with non-virtual locations.

> +source_location
> +linemap_position_for_loc_and_offset (struct line_maps *set,
> +				     source_location loc,
> +				     unsigned int offset)
> +{
> +  const struct line_map * map = NULL;
> +
> +  /* This function does not support virtual locations yet.  */
> +  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
> +
> +  if (offset == 0)
> +    return loc;

Here, I'd replace the above condition and return status statement with:

    if (offset == 0
        /* Adding an offset to a reserved location (like
           UNKNOWN_LOCATION for the C/C++ FEs) does not really make
           sense.  So let's live the location intact in that case.  */
        || loc < RESERVED_LOCATION)
      return loc;

> +
> +  /* First, we find the real location and shift it.  */
> +  loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map);
> +  linemap_assert (MAP_START_LOCATION (map) < loc + offset);

OK.

First I'd add a comment above the assert that says:

/* The new location (loc + offset) should be higher than the first
   location encoded by MAP.  */

and I'd add another assert:

    /* If MAP is not the last line map of its set, then the new location
       (loc + offset) should be less than the first location encoded by
       the next line map of the set.  */
    if (map < LINEMAPS_LAST_ORDINARY_MAP(set))
      linemap_assert(MAP_START_LOCATION(&map[1]) < loc + offset);

> +
> +  offset += SOURCE_COLUMN (map, loc);
> +  linemap_assert (offset < (1u << map->d.ordinary.column_bits));
> +
> +  source_location r = 
> +    linemap_position_for_line_and_column (map,
> +					  SOURCE_LINE (map, loc),
> +					  offset);
> +  linemap_assert (map == linemap_lookup (set, r));
> +  return r;
> +}
> +

OK.

So the line map part of the patch is OK from me if it passes bootstrap
with the added asserts.

Thank you for looking into this.

Cheers.

-- 
		Dodji