Re: [RFA 01/14] Introduce event_location_up

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-04-08 16:11, Tom Tromey wrote:
> This removes make_cleanup_delete_event_location and instead changes
> the various location functions to return an event_location_up, a new
> unique_ptr typedef.

Thanks for doing this, the patch looks good to me.  Two suggestions 
below.

> This is largely straightforward, but be sure to examine the
> init_breakpoint_sal change.  I believe the code I deleted there is
> dead, because "location != NULL" can never be true in that branch; but
> you should double-check.

It looks correct.

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 3925ec6..f3834d5 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3472,7 +3472,7 @@ create_overlay_event_breakpoint (void)
>  				      &internal_breakpoint_ops);
>        initialize_explicit_location (&explicit_loc);
>        explicit_loc.function_name = ASTRDUP (func_name);
> -      b->location = new_explicit_location (&explicit_loc);
> +      b->location = new_explicit_location (&explicit_loc).release ();

Since the breakpoint structure is new'ed and delete'd, you can also 
change breakpoint's location field to be an event_location_up, instead 
of doing a (bunch of) release.  I think you could do the same with the 
location field in linespec_result as well, since that structure is only 
allocated statically (and therefore its defaults ctor/dtor are called).

> @@ -15278,7 +15241,7 @@ static void
>  strace_command (char *arg, int from_tty)
>  {
>    struct breakpoint_ops *ops;
> -  struct event_location *location;
> +  event_location_up location;
>    struct cleanup *back_to;
> 
>    /* Decide if we are dealing with a static tracepoint marker (`-m'),
> @@ -15294,9 +15257,9 @@ strace_command (char *arg, int from_tty)
>        location = string_to_event_location (&arg, current_language);
>      }
> 
> -  back_to = make_cleanup_delete_event_location (location);
> +

You can delete the extra line here.

Thanks,

Simon