From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12197 invoked by alias); 12 Apr 2011 14:39:47 -0000 Received: (qmail 12188 invoked by uid 22791); 12 Apr 2011 14:39:45 -0000 X-SWARE-Spam-Status: No, hits=-5.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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; Tue, 12 Apr 2011 14:39:33 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p3CEdPRu004588 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 12 Apr 2011 10:39:26 -0400 Received: from localhost (ovpn-113-95.phx2.redhat.com [10.3.113.95]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p3CEdNMc007565 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 12 Apr 2011 10:39:25 -0400 Received: by localhost (Postfix, from userid 500) id E52478E604B; Tue, 12 Apr 2011 16:39:22 +0200 (CEST) From: Dodji Seketeli To: Tom Tromey Cc: gcc-patches@gcc.gnu.org, joseph@codesourcery.com, gdr@integrable-solutions.net, lopezibanez@gmail.com Subject: Re: [PATCH 1/6] Linemap infrastructure for virtual locations References: <1291979498-1604-1-git-send-email-dodji@redhat.com> <1291979498-1604-3-git-send-email-dodji@redhat.com> X-URL: http://www.redhat.com Date: Tue, 12 Apr 2011 14:39:00 -0000 In-Reply-To: (Tom Tromey's message of "Thu, 06 Jan 2011 09:24:39 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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-04/txt/msg00894.txt.bz2 Hello, Sorry for getting back to this just now, and thank you very much for the review. Please find below my reply to your comments. Tom Tromey writes: > Dodji> expanded_location xloc; > Dodji> if (loc <= BUILTINS_LOCATION) > Dodji> - { > Dodji> - xloc.file = loc == UNKNOWN_LOCATION ? NULL : _(""); > Dodji> + { > Dodji> + /* If LOC is UNKNOWN_LOCATION and there was a map allocated for > Dodji> + it then XLOC.FILE should be the file path of the map, > Dodji> + i.e. the file path of the main file being compiled. > Dodji> + Otherwise [if we are being called before any line map has > Dodji> + been allocated, e.g. during parsing of commande line > Dodji> + options] if no line map has been allocated yet, then > Dodji> + XLOC.FILE should just be NULL. */ > Dodji> + if (loc == UNKNOWN_LOCATION) > Dodji> + { > Dodji> + const struct line_map *map = > Dodji> + linemap_lookup (line_table, UNKNOWN_LOCATION); > Dodji> + xloc.file = map != NULL ? LINEMAP_FILE (map) : NULL; > Dodji> + } > > I don't understand this. I thought that UNKNOWN_LOCATION should never > have an associated file. This is handled in a funny way: each front end > is responsible for ensuring that this holds true (this could be cleaned > up...). > > Am I mistaken about this? Or is something weird happening after this > patch? I was confused. I realize now this was some papering over what I later realized was PR preprocessor/48532, that I filed separately along with a patch. I have thus removed this hunk from my local copy of the patch. > > Dodji> @@ -883,14 +884,14 @@ static void > Dodji> do_line (cpp_reader *pfile) > Dodji> { > Dodji> const struct line_maps *line_table = pfile->line_table; > Dodji> - const struct line_map *map = &line_table->maps[line_table->used - 1]; > Dodji> + const struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (line_table); > > Dodji> /* skip_rest_of_line() may cause line table to be realloc()ed so note down > Dodji> sysp right now. */ > > Dodji> - unsigned char map_sysp = map->sysp; > Dodji> + unsigned char map_sysp = map->d.ordinary.sysp; > Dodji> const cpp_token *token; > Dodji> - const char *new_file = map->to_file; > Dodji> + const char *new_file = map->d.ordinary.to_file; > > Did you want to use the accessor macros here? > If not, why not? Oops. Yes of course I wanted to use the accessor macros there. I have just done it on my local copy of the patch now. Thanks. > > Dodji> /* C99 raised the minimum limit on #line numbers. */ > Dodji> @@ -945,11 +946,11 @@ static void > Dodji> do_linemarker (cpp_reader *pfile) > Dodji> { > Dodji> const struct line_maps *line_table = pfile->line_table; > Dodji> - const struct line_map *map = &line_table->maps[line_table->used - 1]; > Dodji> + const struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (line_table); > Dodji> const cpp_token *token; > Dodji> - const char *new_file = map->to_file; > Dodji> + const char *new_file = map->d.ordinary.to_file; > Dodji> linenum_type new_lineno; > Dodji> - unsigned int new_sysp = map->sysp; > Dodji> + unsigned int new_sysp = map->d.ordinary.sysp; > > Likewise. > > Dodji> if (map != NULL) > Dodji> - linemap_line_start (pfile->line_table, map->to_line, 127); > Dodji> + linemap_line_start (pfile->line_table, map->d.ordinary.to_line, 127); > > Likewise. > > Dodji> + int sysp = (LINEMAPS_ORDINARY_HIGHEST_LINE (pfile->line_table) > 1) > Dodji> + && (pfile->buffer ? pfile->buffer->sysp : 0); > > The indentation is off here. > Put another paren after the '=' and emacs will DTRT. Done, thanks. > > Dodji> + physicall source locations are called "spelling locations". > > Typo, should be "physical". > > Dodji> +/* A set of chronological line_map structures. */ > Dodji> +struct GTY(()) line_maps { > Dodji> + > Dodji> + struct maps_info info_ordinary; > Dodji> + > Dodji> + struct maps_info info_macro; > > One thing to note is that there may be some code that assumes an > ordering of location values. If you hand out ordinary and macro > locations separately (which I think is what is going on), then code > doing this may break. Argh. The reason why I am doing this is to avoid the (huge) increase of memory usage due to the fact that the presence of a macro map was triggering the creation of one more standard map. Given the number of macros in some TU, that was starting to add up. We have discussed this off-line and a possible way to avoid that was to separate the "integer space" of locations mapped into macro maps from the integer space of locations mapped into ordinary maps. Of course, locations from a given integer space (be it the space of macro tokens locations or ordinary token location) is ordered. This also eases the map lookup as a simple comparisons is enough to know if a given location is of a token coming from a macro expansion or not. Now I think I am left with two options, I guess. Either I find a way to make the two kind of locations (those mapped into macro maps and those mapped into ordinary maps) share the same integer space and be ordered, or, I spot the places in the code that assume an ordering of all location values and I change that. Is the location ordering a strong property we want to keep? > > My recollection is that maybe the diagnostic code did this? > Or maybe it was someplace more obscure :-( > Did you happen to run across this while working on this patch? I haven't noticed that with my tests. But after reading this, I have found one spot of the in diagnostic_report_diagnostic that does this. I don't know yet if there are other places. > > Dodji> - fname = pfile->line_table->maps[pfile->line_table->used-1].to_file; > Dodji> + fname = (LINEMAPS_LAST_ORDINARY_MAP (pfile->line_table))->d.ordinary.to_file; > > Accessor? Oops, fixed in my local copy of the patch. > > Dodji> +/* Add a mapping of logical source line to physical source file and > Dodji> + line number. This function creates an "ordinary map", which is a > Dodji> + map that records locations of tokens that are not part of macro > Dodji> + replacement-lists present at a macro expansion point. > Dodji> + > Dodji> + The text pointed to by TO_FILE must have a lifetime > Dodji> + at least as long as the lifetime of SET. An empty > Dodji> + TO_FILE means standard input. If reason is LC_LEAVE, and > Dodji> + TO_FILE is NULL, then TO_FILE, TO_LINE and SYSP are given their > Dodji> + natural values considering the file we are returning to. > Dodji> + > Dodji> + A call to this function can relocate the previous set of > Dodji> + maps, so any stored line_map pointers should not be used. */ > Dodji> + > Dodji> +const struct line_map * > Dodji> +linemap_add (struct line_maps *set, enum lc_reason reason, > Dodji> + unsigned int sysp, const char *to_file, linenum_type to_line) > > I would generally prefer for explanatory comments for public functions > to be in the header, not by the implementation. This is more in keeping > with the current style of the line map code, but I also like it because > it provides a single text for users of the library to read. Ah. I did this because: - I thought it was in line of the rest of the code of libcpp (and GCC). I didn't realize it was done on purpose in line-map.h only. - Having the documentation be at the place where etags points you to when you look for a function seemed very convenient to me. - It seems to me that having the documentation near the definition helps in keeping the document and the definition in sync. But if you want, I can just move the the comments to the header file, no problem at all. -- Dodji