Re: [PATCH 1/2] gdb: cleanup of displaced_step_inferior_state::reset/displaced_step_clear

[Pedro Alves <palves at redhat dot com>]

On 1/22/20 3:14 PM, Simon Marchi wrote:
> displaced_step_inferior_state::reset and displaced_step_clear appear to
> have the same goal, but they don't do the same thing.
> displaced_step_inferior_state::reset clears more things than
> displaced_step_clear, but it misses free'ing the closure, which
> displaced_step_clear does.
> 
> This patch replaces displaced_step_clear's implementation with just a call to
> displaced_step_inferior_state::reset.  It then changes
> displaced_step_inferior_state::step_closure to be a unique_ptr, to indicate the
> fact that displaced_step_inferior_state owns the closure (and so that it is
> automatically freed when the field is reset).
> 
> It should be possible to get rid of displaced_step_clear entirely, but I'm not
> sure what the best way, give that it's used in scope exit macros.

The reason it needs to be wrapped in a cleanup instead of say SCOPE_EXIT,
is that it needs to be discardable with the "cleanup.release ();" call.
I'm not sure there's a better way.

Renaming displaced_step_clear to displaced_step_reset and adding a comment
may result in clearer code, though.

> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index 8040b28f0172..c6329c844d9b 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -290,7 +290,7 @@ struct displaced_step_inferior_state
>      failed_before = 0;
>      step_thread = nullptr;
>      step_gdbarch = nullptr;
> -    step_closure = nullptr;
> +    step_closure.reset ();

I see people sometimes doing this change and I'm curious.
Is it for clarity?

Anyway, this LGTM.

Thanks,
Pedro Alves