On 06/05/2014 10:44 PM, Doug Evans wrote:

> Alas I've been told we're no longer allowed to add new make_cleanup_foo
> functions.
> [Can't remember when, but I'm trusting my memory on this one.  :-)]
> OTOH there's nothing written down in the coding standards section of the wiki.
> I think the loss of type safety isn't warranted,
> so I'm going to approve this part, and see if anyone complains
> (or wants you to do things differently).

I guess I'll wait for someone to complain, then! :-P

>>>    > +
>>>    > +struct event_location
[snip]
>>>    > +};
>>>
>>> Making this struct opaque to its users has benefits.
>>> Thoughts?
>>
>> I briefly considered this but didn't see an real benefits. Can you
>> elaborate? If you feel that strongly about it, I can change it.
>
> Making structs opaque (where there's a choice) helps keep things
> maintainable. e.g., only the implementation details one wants to expose
> actually do get exposed.  If it's a toss up, or close to one, I'd
> go the opaque struct route.  The patch is already providing accessors,
> so why not make those accessors functions instead of macros?
> A teensy bit more to write, but I think it's worth it.
> [OTOH I haven't seen the rest of the patch set yet,
> if it proves excessively onerous I'm happy to revisit.]

Ok, in this next revision, I've changed this structure to be completely 
opaque...

So instead of doing:

struct location *location, copy_location;

location = string_to_event_location (&arg, current_language);
copy_location = *location;
decode_line_full (&copy_location, ...);
if (event_location_type (location) == LINESPEC_LOCATION)
      arg = get_linespec_location (&copy_location); /* to advance ARG 
over processed input */

the code now does:

struct event_location *location, *copy_location;

location = string_to_event_location (&arg, current_language);
copy_location = copy_event_location_tmp (location);
make_cleanup (xfree, copy_location);
decode_line_full (copy_location, ...);
event_location_advance_input_ptr (&arg, copy_location);
do_cleanups (...);

> Within location.c I would just access the struct members directly,
> instead of with accessor macros.
> [I know accessor macros is a GNU-ism, but we don't always use them
> in dwarf2read.c (e.g.,: v.quick), and do just fine IMO.]

Well, I kept my accessor macros in location.c (and nowhere else)... I 
like them, but I am not married to them. If you/others really want me to 
remove them, just say the word, and I'll zap 'em.

>>>    > +/* Return a string representation of the LOCATION.
>>>    > +   This function may return NULL for unspecified linespecs,
>>>    > +   e.g, EVENT_LOCATION_LINESPEC and addr_string is NULL.  */
>>>    > +
>>>    > +extern const char *
>>>    > +  event_location_to_string (const struct event_location *location);
>>>
>>> I wonder if the string form might be computed
>>> for some kind of location.  In which case either we always
>>> return a malloc'd copy here (remove const from result)
>>> or lazily compute it and cache the computed string in the
>>> struct (remove the const from the location parameter).
>>> [Or switch to c++ and use mutable I guess. :)]

As I mentioned the last time around, I've changed the event location API 
to compute this string when it can (it caches a copy of it inside the 
struct event_location -- I'm seeing this string computed many, many times).

There are two instances in which we save the string instead of computing it:

1) pending breakpoints: we save the entire input string into the 
location (needed to preserve, e.g., conditions)

2) When converting/canonicalizing a linespec to explicit form, we must 
save the linespec string representation (so that we can reproduce the 
linespec the way the user originally specified it). The other option 
here is to add a flag to event_location to note this. Then we can 
compute the location. I chose to simply save the linespec string 
representation.

Updated patch attached.

Keith

Changes since last revision:
     - Move struct event_location to location.c
     - Make struct event_location completely opaque
     - Turn all EVENT_LOCATION_* macros to functions.
     - Add get_linespec_location, copy_event_location_tmp,
       event_location_advance_input_ptr,
       advance_event_location_ptr, set_event_location_string
     - Remove duplicate function comments/"See description in foo.h."