From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13171 invoked by alias); 4 Aug 2011 21:30:04 -0000 Received: (qmail 13091 invoked by uid 22791); 4 Aug 2011 21:30:02 -0000 X-SWARE-Spam-Status: No, hits=-6.8 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; Thu, 04 Aug 2011 21:29:44 +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 p74LTDht007912 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 4 Aug 2011 17:29:14 -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 p74LTCXb020139; Thu, 4 Aug 2011 17:29:13 -0400 Received: from [0.0.0.0] (ovpn-113-87.phx2.redhat.com [10.3.113.87]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p74LT9iL013262; Thu, 4 Aug 2011 17:29:09 -0400 Message-ID: <4E3B0F24.90708@redhat.com> Date: Thu, 04 Aug 2011 21:30: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> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; 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/msg00538.txt.bz2 On 08/04/2011 11:27 AM, Dodji Seketeli wrote: > Yes, Tom and I thought about that, and decided that, this could be an > optimization that could add later when the whole thing is known to > work. Makes sense. > +/* This is the highest possible source location encoded within an > + ordinary or macro map. */ > +#define MAX_SOURCE_LOCATION 0xF0000000 Why not 0xFFFFFFFF? I'm not sure what the rationale for using that value here: > /* If the column number is ridiculous or we've allocated a huge > number of source_locations, give up on column numbers. */ > max_column_hint = 0; > - if (highest >0xF0000000) > + if (highest > MAX_SOURCE_LOCATION) > return 0; was, but I would think that we would be fine to use that upper range for macro maps. > - size_t to_file_len = strlen (map->to_file); > + const char *file_path = LOCATION_FILE (src_loc); > + int sysp; > + size_t to_file_len = strlen (file_path); > unsigned char *to_file_quoted = > (unsigned char *) alloca (to_file_len * 4 + 1); > unsigned char *p; > > - print.src_line = SOURCE_LINE (map, src_loc); > - print.src_file = map->to_file; > + print.src_line = LOCATION_LINE (src_loc); > + print.src_file = LOCATION_FILE (src_loc); It probably doesn't matter much, but you could just do print.src_line = file_path; to avoid an extra expand_location. Or have an expanded_location variable here like you do in fortran/cpp.c. > + return ((location - MAP_START_LOCATION (map)) > + >> ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) > + + ORDINARY_MAP_STARTING_LINE_NUMBER (map); This should use SOURCE_LINE. > + return (location - MAP_START_LOCATION (map)) > + & ((1 << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) - 1); And SOURCE_COLUMN. > - means "entire file/line" or "unknown line/column" or "not applicable".) > - INCLUDED_FROM is an index into the set that gives the line mapping > - at whose end the current one was included. File(s) at the bottom > - of the include stack have this set to -1. REASON is the reason for > - creation of this line map, SYSP is one for a system header, two for > - a C system header file that therefore needs to be extern "C" > - protected in C++, and zero otherwise. */ > -struct GTY(()) line_map { > + means "entire file/line" or "unknown line/column" or "not > + applicable".) > + > + The highest possible source location is MAX_SOURCE_LOCATION > +*/ Looks like you didn't need to rewrap the "entire file" line. Also, the trailing */ goes on the last line of text, not on a line by itself. > - most recent linemap_add). MAX_COLUMN_HINT is the highest column > + most recent linemap_add). MAX_COLUMN_HINT is the highest column Unnecessary reformatting. > > +bool > +linemap_tracks_macro_expansion_locs_p (struct line_maps *set) > +{ > + return LINEMAPS_MACRO_MAPS (set) != NULL; > +} > + > +const struct line_map * > +linemap_enter_macro (struct line_maps *set, struct cpp_macro *macro, > + source_location expansion, unsigned int num_tokens) Two of the several functions without comments. You can just copy the comments from the header in here. > +#define linemap_assert(EXPR) \ > + do { \ > + if (! (EXPR)) \ > + abort (); \ > + } while (0) This should be controlled by ENABLE_CHECKING. > +int > +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. > +bool > +linemap_location_from_macro_expansion_p (struct line_maps *set, > + source_location location) > +{ > + linemap_assert (location <= MAX_SOURCE_LOCATION); > + if (set == NULL) > + return false; > + return (location > set->highest_location); > +} We need to make sure that set->highest_location < LINEMAPS_MACRO_LOWEST_LOCATION, either here or anywhere that changes one of those two values. Jason