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

[Jason Merrill <jason@redhat.com>]

On 09/20/2011 10:05 AM, Dodji Seketeli wrote:
> Jason Merrill<jason@redhat.com>  writes:

>> In linemap_macro_map_loc_to_def_point you get the token number and
>> then use that to index into MACRO_MAP_LOCATIONS.  Can't you use the
>> same token number to index into macro->exp.tokens instead?

> No, because a macro argument can be made of several tokens.

Ah, I see.

>> I don't understand how that unwinding operation maps onto a function
>> called linemap_macro_map_loc_to_def_point,

> In the example I used in the comment of that function, suppose that
> during 'stepping out', as you put it, we don't deal with the place in
> the source where '<<' appears in the definition of a macro.  We'd then
> only deal with the expansion points of the macros involved.

I think I wasn't expressing clearly enough the point I was trying to 
make.  My point was more that the name of the function is confusing; if 
what you get back is another virtual location, that's not what I would 
consider a "def point".  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".  And so when you go to print actual diagnostics you use 
LRK_MACRO_PARM_REPLACEMENT_POINT.

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.

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?  In the example, 
once we hit << in SHIFTL unwinding needs to switch to the expansion point.

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));

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), 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).

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.

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.

What do you think?

Jason