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

Pedro Alves
2016-10-19T23:58:06Z

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);
>> \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)?

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