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 <do...@redhat.com> writes: > 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