Re: [PATCH 1/6] Linemap infrastructure for virtual locations

[Tom Tromey <tromey@redhat.com>]

>>>>> "Dodji" == Dodji Seketeli <dodji@redhat.com> writes:

Dodji> This is the first instalment of a set which goal is to track locations
Dodji> of tokens across macro expansions. Tom Tromey did the original work
Dodji> and attached the patch to PR preprocessor/7263. This opus is a
Dodji> derivative of that original work.

Thank you very much for doing this.

I think the accessor macros and changes to use them could go in as a
separate patch.  It doesn't matter too much now, but I think this would
have made this patch more readable.

Dodji>    expanded_location xloc;
Dodji>    if (loc <= BUILTINS_LOCATION)
Dodji> -    {
Dodji> -      xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
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?

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?

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.

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.

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?

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?

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.

Nice job on the comments, btw.

Tom