Hello Manuel,
Manuel López-Ibáñez <lopeziba...@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 <m...@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
