Re: [PATCH 3/7] Emit macro expansion related diagnostics

[Dodji Seketeli <dodji at redhat dot com>]

Jason Merrill <jason@redhat.com> writes:

> My point was more that the name of the function is confusing;

Sorry about that.

> if what you get back is another virtual location, that's not what I
> would consider a "def point".

For tokens that are not function-like macro arguments, I think the
linemap_macro_loc_to_def_point makes sense because what we get then is
the source location in the actual definition of the macro.  Actually I
think that's where the confusion comes from.  I first devised the name
while thinking of the case of macros that are not function-like.  And
now it falls short for the general case.  I should have caught that
but for some reason I was just seeing through the name is if it was
transparent.  Sigh.

I take your point; that name doesn't make sense in the general case.

> The only time you get a source location in the actual definition of
> the macro is when you ask for the "macro parm usage point".

Yes that, and in the case above; in which case xI and yI are equal, by
the way.

> When we start out, we have a virtual location that represents << in
> the expansion of OPERATE.  Then we call
> linemap_macro_map_loc_to_def_point to get a virtual location that
> represents << in the expansion of SHIFTL. This is not part of the
> definition of OPERATE, and shouldn't be described as such.

Agreed.

> It seems that this function steps out until we hit the spelling
> location, and then we have a real source location and stop, which is
> why the loop in maybe_unwind_expanded_macro_loc needs to use
> linemap_macro_map_loc_to_exp_point as well.  Right?

Right.

> It seems to me that we should encapsulate all of this in a function
> that expresses this unwinding operation, say
> "linemap_macro_map_loc_unwind_once".  So the loop would look like
> 
> +  do
> +    {
> +      loc.where = where;
> +      loc.map = map;
> +
> +      VEC_safe_push (loc_t, heap, loc_vec, &loc);
> +
> +      /* WHERE is the location of a token inside the expansion of a
> +         macro.  MAP is the map holding the locations of that macro
> +         expansion.  Let's get the location of the token in the
> +         context that triggered the expansion of this macro.
> +         This is basically how we go "down" in the trace of macro
> +         expansions that led to WHERE.  */
> +      where = linemap_unwind_once (where, &map);
> +    } while (linemap_macro_expansion_map_p (map));
> 

OK.

> I think that linemap_macro_loc_to_def_point when called with false
> returns the unwound spelling location of the token (and thus is used
> for LRK_SPELLING_LOCATION),

Right.

> or when called with true returns the (not-unwound) location in the
> expanded macro (and so I think LRK_MACRO_PARM_REPLACEMENT_POINT
> should be renamed to LRK_MACRO_EXPANDED_LOCATION or something
> similar).

FWIW, I'd like the LRK_MACRO_PARM_REPLACEMENT name (or its
replacement.  ha ha) to hint at the fact that it really has to do with
a token that is an /argument/ for a function-like macro.  So maybe
LRK_MACRO_PARM_FOR_ARG_LOCATION?  LRK_MACRO_EXPANDED_LOCATION really
seems too vague to me.  After all, pretty much everything is about
*EXPAND*ing macros here.  :-)

> It seems to me that either we should split those functions apart in
> to two functions with clearer names, or bundle them and
> linemap_macro_loc_to_exp_point into linemap_resolve_location; if
> linemap_location_in_system_header_p used linemap_resolve_location it
> would be more readable anyway.

OK.

> I'm having trouble thinking of a less misleading name for
> linemap_macro_map_loc_to_def_point, since the two locations serve
> completely different purposes.  Maybe something vague like
> linemap_macro_map_loc_get_stored_loc.  Or split it into two
> functions like linemap_virtual_loc_unwind_once_toward_spelling and
> linemap_virtual_loc_get_expanded_location or something like that.

So would a function named linemap_macro_map_loc_to_def_point that
returns the second location (yI) only, and a second function
linemap_macro_map_loc_unwind_once be less confusing to you?  If yes,
then I'll send an updated patch for that in a short while.

Thanks.

-- 
		Dodji