Jason Merrill <ja...@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
