From: Dodji Seketeli <dodji@redhat.com>
To: "Manuel López-Ibáñez" <lopezibanez@gmail.com>
Cc: Gcc Patch List <gcc-patches@gcc.gnu.org>,
"fortran\@gcc.gnu.org List" <fortran@gcc.gnu.org>,
Tobias Burnus <burnus@net-b.de>
Subject: Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C
Date: Thu, 23 Oct 2014 19:12:00 -0000 [thread overview]
Message-ID: <871tpylhzc.fsf@redhat.com> (raw)
In-Reply-To: <87iojbkpmb.fsf@redhat.com> (Dodji Seketeli's message of "Thu, 23 Oct 2014 13:11:08 +0200")
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
next prev parent reply other threads:[~2014-10-23 19:10 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-16 21:28 Manuel López-Ibáñez
2014-10-17 7:25 ` Tobias Burnus
2014-10-23 11:14 ` Dodji Seketeli
2014-10-23 19:12 ` Dodji Seketeli [this message]
2014-10-23 22:48 ` Tobias Burnus
2014-10-24 16:18 ` Manuel López-Ibáñez
2014-11-28 16:55 ` Manuel López-Ibáñez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871tpylhzc.fsf@redhat.com \
--to=dodji@redhat.com \
--cc=burnus@net-b.de \
--cc=fortran@gcc.gnu.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=lopezibanez@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).