From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7368 invoked by alias); 23 Oct 2014 19:10:57 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 7342 invoked by uid 89); 23 Oct 2014 19:10:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 23 Oct 2014 19:10:56 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9NJAqAS012358 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 23 Oct 2014 15:10:53 -0400 Received: from localhost (ovpn-116-99.ams2.redhat.com [10.36.116.99]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9NJAmVH013527 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 23 Oct 2014 15:10:50 -0400 Received: by localhost (Postfix, from userid 1000) id 8E5EE412B1; Thu, 23 Oct 2014 21:10:47 +0200 (CEST) From: Dodji Seketeli To: Manuel =?utf-8?B?TMOzcGV6LUliw6HDsWV6?= Cc: Gcc Patch List , "fortran\@gcc.gnu.org List" , Tobias Burnus Subject: Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C References: <87iojbkpmb.fsf@redhat.com> X-URL: http://www.redhat.com Date: Thu, 23 Oct 2014 19:12:00 -0000 In-Reply-To: <87iojbkpmb.fsf@redhat.com> (Dodji Seketeli's message of "Thu, 23 Oct 2014 13:11:08 +0200") Message-ID: <871tpylhzc.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2014-10/txt/msg02445.txt.bz2 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 writes: > Hello Manuel, > > Manuel L=C3=B3pez-Ib=C3=A1=C3=B1ez writes: > > >> Dodji, are the linemap_asserts() appropriate? > > Yes they are. I have some additional comments though. > >> libcpp/ChangeLog: >> >> 2014-10-16 Manuel L=C3=B3pez-Ib=C3=A1=C3=B1ez >> >> 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); >>=20=20 >> +/* 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 =3D NULL; >> + >> + /* This function does not support virtual locations yet. */ >> + linemap_assert (!linemap_location_from_macro_expansion_p (set, loc)); >> + >> + if (offset =3D=3D 0) >> + return loc; > > Here, I'd replace the above condition and return status statement with: > > if (offset =3D=3D 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 =3D linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &m= ap); >> + 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 +=3D SOURCE_COLUMN (map, loc); >> + linemap_assert (offset < (1u << map->d.ordinary.column_bits)); >> + >> + source_location r =3D=20 >> + linemap_position_for_line_and_column (map, >> + SOURCE_LINE (map, loc), >> + offset); >> + linemap_assert (map =3D=3D 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. --=20 Dodji