Re: [PATCH 05/16 v2] GDBserver clone breakpoint list

["Breazeal, Don" <donb at codesourcery dot com>]

On 10/15/2014 10:40 AM, Pedro Alves wrote:
> On 08/21/2014 01:29 AM, Don Breazeal wrote:
>> This patch implements gdbserver routines to clone the breakpoint lists of a
>> process, duplicating them for another process.

>> +#define APPEND_TO_LIST(listpp, itemp, tailp) \
>> +	 { \
>> +	   if (tailp == NULL) \
>> +	     *(listpp) = itemp; \
>> +	   else \
>> +	     tailp->next = itemp; \
>> +	   tailp = itemp; \
>> +	 }
> 
> Please put ()'s around uses of the parameters.
> Also, use 'do { ... } while (0)' instead of '{ ... }'.

Done.

>> +static void
>> +clone_agent_expr (struct agent_expr **new_ax, struct agent_expr *src_ax)
>> +{

> 
> Like memcpy, please make the src argument of these functions be
> const.

Done.

> 
> Is there a reason this doesn't have the more natural prototype that
> returns the new clone?

No reason.  I've made this change in the new version.

> Here this could then be:
> 
>       new_bkpt = clone_one_breakpoint (bp);
>       APPEND_TO_LIST (new_list, new_bkpt, bkpt_tail);
>       APPEND_TO_LIST (new_raw_list, new_bkpt->raw, raw_bkpt_tail);
> 
>> +/* Create a new breakpoint list NEW_BKPT_LIST that is a copy of SRC.  */
>> +
>> +void clone_all_breakpoints (struct breakpoint **new_bkpt_list,
>> +			    struct raw_breakpoint **new_raw_bkpt_list,
>> +			    struct breakpoint *src);

And this change is made as well.

> 
> It took me a second to realize that SRC is a list here rather than
> a single breakpoint.  Could you make that more explicit, like e.g.,:
> 
> /* Create a new breakpoint list NEW_BKPT_LIST that is a copy of the
>    list that starts at SRC.  */

I made this change, and also changed the name of SRC to SRC_LIST.

Thanks for the review.  I've posted an updated version of this patch
here: https://sourceware.org/ml/gdb-patches/2014-10/msg00870.html.
It's part of a new version of the whole patch series, updated to work
with more recent changes.

Thanks,
--Don