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

[Jason Merrill <jason at redhat dot com>]

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