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

[Pedro Alves <palves at redhat dot com>]

On 10/20/2016 12:19 AM, Simon Marchi wrote:
> 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)?

m_ is supposed to used for private data fields.  In this case,
the fields are still public (and referenced from outside the class
in many places).  I think in such cases using this-> makes the code
much clearer.

> 
>> @@ -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.

Hmm, I saw the "Free" and thought that since we now use "delete", release
would be a generic term that would be clearer.  But you have a good point.
Free -> Delete ?  Or I can just keep it was Free.

Thanks,
Pedro Alves