From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20299 invoked by alias); 19 Aug 2011 04:05:16 -0000 Received: (qmail 20283 invoked by uid 22791); 19 Aug 2011 04:05:11 -0000 X-SWARE-Spam-Status: No, hits=-7.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 19 Aug 2011 04:04:50 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7J44JTo019366 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 19 Aug 2011 00:04:19 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p7J44Iil030360; Fri, 19 Aug 2011 00:04:18 -0400 Received: from [0.0.0.0] (ovpn-113-53.phx2.redhat.com [10.3.113.53]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p7J44EGb007365; Fri, 19 Aug 2011 00:04:15 -0400 Message-ID: <4E4DE0BE.2020902@redhat.com> Date: Fri, 19 Aug 2011 08:46:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20110719 Thunderbird/5.0 MIME-Version: 1.0 To: Dodji Seketeli CC: gcc-patches@gcc.gnu.org, tromey@redhat.com, gdr@integrable-solutions.net, joseph@codesourcery.com, burnus@net-b.de, charlet@act-europe.fr, bonzini@gnu.org Subject: Re: [PATCH 1/7] Linemap infrastructure for virtual locations References: <1291979498-1604-1-git-send-email-dodji@redhat.com> <3d6fbfd16f0e3493839205de1266eaaa8dbb9c77.1310824121.git.dodji@redhat.com> <4E378168.2090903@redhat.com> <4E3B0F24.90708@redhat.com> <4E3C2796.7070708@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1251; format=flowed Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2011-08/txt/msg01545.txt.bz2 On 08/09/2011 10:31 AM, Dodji Seketeli wrote: > -#define in_system_header (in_system_header_at (input_location)) > +#define in_system_header (in_system_header_at (input_location)) Unnecessary whitespace change. > struct GTY(()) cpp_macro { > + /* Name of this macro. Used only for error reporting. */ > + cpp_hashnode * GTY ((nested_ptr (union tree_node, > + "%h ? CPP_HASHNODE (GCC_IDENT_TO_HT_IDENT (%h)) : NULL", > + "%h ? HT_IDENT_TO_GCC_IDENT (HT_NODE (%h)) : NULL"))) > + name; Would it make sense to have line_map_macro store a pointer to the cpp_hashnode rather than straight to the cpp_macro so we don't have to add an extra pointer to cpp_macro? > +struct GTY(()) cpp_macro; I don't think we need GTY markers on forward declarations. > + LC_ENTER_MACRO > + /* stringize */ > + /* paste */ What is the purpose of these comments? > + - when a new preprocessing unit start. > + - when a preprocessing unit ends. > + - when a macro expansion occurs > + */ */ should go at the end of the line. > +/* > + Create a macro map. A macro map encodes source locations of tokens And /* at the beginning of the line. >>> +linemap_check_ordinary (const struct line_map *map) >>> +{ >>> + linemap_assert (!linemap_macro_expansion_map_p (map)); >>> + /* Return any old value. */ >>> + return 0; >>> +} >> Why does this return int if we aren't interested in the value? I >> would change it to a macro that returns 'map' so that it can expand to >> nothing if !ENABLE_CHECKING and can be used in-line like the gcc >> TREE_CHECK macros. >> > ... this. This is now implemented by a macro as you are suggesting. You changed it to a macro that returns 'map' but didn't change the uses to inline, i.e. > + (linemap_check_ordinary (MAP), (MAP)->d.ordinary.to_file) to + (linemap_check_ordinary (MAP)->d.ordinary.to_file) For that to work, the !ENABLE_CHECKING version also needs to return MAP. If you aren't going to change the users, the ENABLE_CHECKING version might as well not return anything. > +typedef struct GTY (()) > +{ The GTY(()) can't work here without a tag name associated with it. And we never gc-allocate extended_location, so it isn't needed. > +enum location_resolution_kind > +{ > + LRK_MACRO_EXPANSION_POINT, > + LRK_SPELLING_LOCATION, > + LRK_MACRO_PARM_REPLACEMENT_POINT > +}; Let's move this after the comment that explains the values. > + > > -static void trace_include (const struct line_maps *, const struct line_map *); > > +static void trace_include (const struct line_maps *, const struct line_map *); Unnecessary change. > + /* If we don't keep our line maps consistent, we can easily > + segfault. Don't rely on the client to do it for us. */ This sounds like working around a bug. Wouldn't it be better to fix it? > So this is a second try. I have removed that test altogether. But > then I figured I should nonetheless handle the case where we run out > of integer space when allocating locations for both ordinary and macro > tokens. So linemap_line_start hands out 0 in that case (for ordinary > tokens) and linemap_enter_macro hands out NULL in lieu of a macro map > in that case. That NULL map is handled (in the second patch of the > series) so that the spelling location of the macro token is used in > that case. Is something still protecting from numeric overflow in the case that there aren't any macros? I'd leave that test the way it was before (and keep your new tests too). > + /* If LOCATION is reserved for the user of libcpp, it means, > + e.g. for gcc that it's the location of a built-in token. In that > + case, let's say that the final location is the macro expansion > + point because otherwise, the built-in location would not make > + any sense and would violate the invariant that says that every > + single location must be >= to the MAP_START_LOCATION (MAP) of its > + map. */ How can this happen? Can't we just assert that it doesn't, like we do in the next function? Jason