Re: [PATCH v2 29/31] 'struct agent_expr *' -> unique_ptr<agent_expr>

Simon Marchi
2016-10-19T23:19:24Z

On 2016-10-18 21:12, Pedro Alves wrote:
> diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> index 7f27a45..35225f6 100644
> --- a/gdb/ax-general.c
> +++ b/gdb/ax-general.c
> @@ -37,52 +37,30 @@ static void generic_ext (struct agent_expr *x,
> enum agent_op op, int n);
> \f
> /* Functions for building expressions. */
>
> -/* Allocate a new, empty agent expression. */
> -struct agent_expr *
> -new_agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
> +agent_expr::agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
> {
> - struct agent_expr *x = XNEW (struct agent_expr);
> -
> - x->len = 0;
> - x->size = 1; /* Change this to a larger value once
> + this->len = 0;
> + this->size = 1; /* Change this to a larger value once
> reallocation code is tested. */
> - x->buf = (unsigned char *) xmalloc (x->size);
> + this->buf = (unsigned char *) xmalloc (this->size);
>
> - x->gdbarch = gdbarch;
> - x->scope = scope;
> + this->gdbarch = gdbarch;
> + this->scope = scope;
>
> /* Bit vector for registers used. */
> - x->reg_mask_len = 1;
> - x->reg_mask = XCNEWVEC (unsigned char, x->reg_mask_len);
> -
> - x->tracing = 0;
> - x->trace_string = 0;
> + this->reg_mask_len = 1;
> + this->reg_mask = XCNEWVEC (unsigned char, this->reg_mask_len);
>
> - return x;
> + this->tracing = 0;
> + this->trace_string = 0;
> }

In one of Tom's patches, you said to drop the "this->". Did you leave
them here for clarity? Would you remove them if the structure was
completely converted, with m_ prefixed members (given that the m_ prefix
makes it clear enough that it's a member)?

> @@ -12385,13 +12378,9 @@ force_breakpoint_reinsertion (struct
> bp_location *bl)
> that have already been marked. */
> loc->condition_changed = condition_updated;
>
> - /* Free the agent expression bytecode as well. We will compute
> - it later on. */
> - if (loc->cond_bytecode)
> - {
> - free_agent_expr (loc->cond_bytecode);
> - loc->cond_bytecode = NULL;
> - }
> + /* Release the agent expression bytecode as well. We will
> + compute it later on. */
> + loc->cond_bytecode.reset ();

Why did you change Free for Release in the comment? Since release has a
different meaning when using unique pointers, it sounds more confusing
like this I think.