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

[Dodji Seketeli <dodji at redhat dot com>]

Sorry, I forgot to make it clear that I have no power to approve the
line-map changes.  I just gave my casual point view; so please take it
for what it is worth.

I am CC-ing Tom and Jason who can approve this.

Cheers.

Dodji Seketeli <dodji@redhat.com> writes:

> 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