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

[Jason Merrill <jason@redhat.com>]

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