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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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);
>  
>  /* 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.