Re: [PATCH 4/4] Replace line_map union with C++ class hierarchy

[Jeff Law <law at redhat dot com>]

On 05/04/2015 03:27 PM, David Malcolm wrote:
> > > @@ -158,7 +158,7 @@ expand_location_1 (source_location loc,
> > >    	     location (toward the expansion point) that is not reserved;
> > >    	     that is, the first location that is in real source code.  */
> > >    	  loc = linemap_unwind_to_first_non_reserved_loc (line_table,
> > > -							  loc, &map);
> > > +							  loc, NULL);
> >
> > Is that really correct?
>
>
> I believe so.  The old code had (in gcc/input.c's expand_location_1):
[ ... ]
THanks for walking through it.  Mostly it just stuck out as "odd".

Given that we're depending on "not read" behaviour and latter calls 
writing into that object again (thus making the original object in 
memory "dead"), it'd be good to try and clean this up a little.

You could argue that passing in NULL like you did *is* actually that 
cleanup and I'd probably agree with the hesitation that for it to be a 
valid cleanup, linemap_resolve_location must never read what's in that 
location.

linemap_resolve_location's function header indicates it's an output, so 
I think we're OK.

So, concern eliminated :-)


> >     /* Return the map at a given index.  */
> > >    inline line_map *
> > >    LINEMAPS_MAP_AT (const line_maps *set, bool map_kind, int index)
> > >    {
> > > -  return &(LINEMAPS_MAPS (set, map_kind)[index]);
> > > +  if (map_kind)
> > > +    return &set->info_macro.maps[index];
> > > +  else
> > > +    return &set->info_ordinary.maps[index];
> > >    }
> > I see this pattern repeated regularly.  Any thoughts on how painful
> > it'll be to drop the map_kind argument and instead not vary the kind?
>
> I'm not sure I understand your question.
Given that kind of code, I'd be looking at the callers to determine if 
there's a reasonable way to bubble up the if (map_kind) test.

This is particularly good if bubbling it up ultimately results in 
somewhere in the call chain a statically determined map_kind.

I don't consider resolving this a requirement to go forward and it looks 
like you tried implementing this stuff with templates and ran afoul of 
gengtype.  Fair enough :-0

So I think at this point the patch is good to go.

Jeff