Re: [PATCH] change the arguments of create_breakpoint to a struct

Hi Sergio,

Thanks for your review.

On Thu, Feb 21, 2013 at 9:58 PM, Sergio Durigan Junior
<[email protected]> wrote:
> On Thursday, February 21 2013, Hui Zhu wrote:
>
>> According to the discussion in
>> http://sourceware.org/ml/gdb/2012-08/msg00001.html
>>
>> I post a new patch for GDB to change the arguments of
>> create_breakpoint to a struct.
>> And I also add a function create_breakpoint to Initialize all the
>> element of this struct to its default value.  Then if we want to add
>> more argument to this function, we can just add the default value
>> Initialize code to this function and don't need update all the
>> function that call create_breakpoint.
>>
>> And I post a patch for insight to change the arguments of
>> create_breakpoint to a struct.
>>
>> Please help me review it.
>
> Thanks for the patch, Hui.  A few comments.
>
>> --- a/breakpoint.c
>> +++ b/breakpoint.c
>> @@ -9493,32 +9493,27 @@ decode_static_tracepoint_spec (char **ar
>>    return sals;
>>  }
>>
>> +/* Initialize all the element of CB to its default value. */
>
> I think it is better to write "... to their default values."  Also
> missing double-space after period.
>
>> +void
>> +create_breakpoint_init (struct create_breakpoint_s *cb)
>> +{
>> +  memset (cb, '\0', sizeof (*cb));
>> +  cb->type_wanted = bp_breakpoint;
>> +  cb->pending_break_support = AUTO_BOOLEAN_TRUE;
>> +  cb->enabled = 1;
>> +}
>
> I would change the name of the fuction to `init_breakpoint_properties'
> or something (see more below).
>
> I have read the discussion on the link you posted above, but still I
> want to confirm something: would it be possible to make this function
> take all the arguments that create_breakpoint et al now take?  My point
> is that, IMO, it would be clearer to pass all those arguments to this
> function directly, instead of filing them in the respective callers.
> Matter of taste, of couse.

Cool!  That is a good part to discussion.  When I planed to do this
patch, I thought about let this function do it.
I thought is If move all the argument of create_breakpoint to this
function.  Then this function will become another create_breakpoint.
Then I thought is maybe we can select some arguments that don't have
default value.  But it need more discussion on that.

And about cleaning up, I think is is good comments too.  I thought is
maybe we can do both of them: update arguments to a struct and
cleaning up them.

I am sorry that I didn't post a new patch because I think maybe we
need more discussion on this issue.

Thanks,
Hui

>
>>  /* Set a breakpoint.  This function is shared between CLI and MI
>> -   functions for setting a breakpoint.  This function has two major
>> -   modes of operations, selected by the PARSE_CONDITION_AND_THREAD
>> -   parameter.  If non-zero, the function will parse arg, extracting
>> -   breakpoint location, address and thread.  Otherwise, ARG is just
>> -   the location of breakpoint, with condition and thread specified by
>> -   the COND_STRING and THREAD parameters.  If INTERNAL is non-zero,
>> -   the breakpoint number will be allocated from the internal
>> -   breakpoint count.  Returns true if any breakpoint was created;
>> -   false otherwise.  */
>> +   functions for setting a breakpoint.
>> +   Returns true if any breakpoint was created; false otherwise.  */
>
> You have changed only the function prototype and they way the function
> accesses its arguments, but not the function behaviour itself, so I
> would be against changing this comment.  Maybe it could be rewritten to
> inform that the fields are now part of the CB struct.
>
>>  int
>> -create_breakpoint (struct gdbarch *gdbarch,
>> -                char *arg, char *cond_string,
>> -                int thread, char *extra_string,
>> -                int parse_condition_and_thread,
>> -                int tempflag, enum bptype type_wanted,
>> -                int ignore_count,
>> -                enum auto_boolean pending_break_support,
>> -                const struct breakpoint_ops *ops,
>> -                int from_tty, int enabled, int internal,
>> -                unsigned flags)
>> +create_breakpoint (struct create_breakpoint_s *cb)
>>  {
>
> [...]
>
>> @@ -9743,6 +9744,7 @@ break_command_1 (char *arg, int flag, in
>>                            : bp_breakpoint);
>>    struct breakpoint_ops *ops;
>>    const char *arg_cp = arg;
>> +  struct create_breakpoint_s cb;
>>
>>    /* Matching breakpoints on probes.  */
>>    if (arg && probe_linespec_to_ops (&arg_cp) != NULL)
>> @@ -9750,17 +9752,16 @@ break_command_1 (char *arg, int flag, in
>>    else
>>      ops = &bkpt_breakpoint_ops;
>>
>> -  create_breakpoint (get_current_arch (),
>> -                  arg,
>> -                  NULL, 0, NULL, 1 /* parse arg */,
>> -                  tempflag, type_wanted,
>> -                  0 /* Ignore count */,
>> -                  pending_break_support,
>> -                  ops,
>> -                  from_tty,
>> -                  1 /* enabled */,
>> -                  0 /* internal */,
>> -                  0);
>> +  create_breakpoint_init (&cb);
>> +  cb.gdbarch = get_current_arch ();
>> +  cb.arg = arg;
>> +  cb.parse_condition_and_thread = 1;
>> +  cb.tempflag = tempflag;
>> +  cb.type_wanted = type_wanted;
>> +  cb.pending_break_support = pending_break_support;
>> +  cb.ops = ops;
>> +  cb.from_tty = from_tty;
>> +  create_breakpoint (&cb);
>
> Just to make my argument clearer, my suggestion is to actually replace
> this with:
>
>
>     init_breakpoint_properties (&cb, get_current_arch (), arg,
>                                 ... );
>     create_breakpoint (&cb);
>
> Don't know, it seems more organized to me.  But of course, you might
> just want to wait for some maintainer to give his suggestion :-).
>
>> --- a/breakpoint.h
>> +++ b/breakpoint.h
>> @@ -1265,17 +1265,79 @@ enum breakpoint_create_flags
>>      CREATE_BREAKPOINT_FLAGS_INSERTED = 1 << 0
>>    };
>>
>> -extern int create_breakpoint (struct gdbarch *gdbarch, char *arg,
>> -                           char *cond_string, int thread,
>> -                           char *extra_string,
>> -                           int parse_condition_and_thread,
>> -                           int tempflag, enum bptype wanted_type,
>> -                           int ignore_count,
>> -                           enum auto_boolean pending_break_support,
>> -                           const struct breakpoint_ops *ops,
>> -                           int from_tty,
>> -                           int enabled,
>> -                           int internal, unsigned flags);
>> +/* This argument struct for function CREATE_BREAKPOINT.  */
>
> May I suggest rewritting this comment?  Something like:
>
> /* Structure which holds the properties of breakpoints and their
>    variations (tracepoints, watchpoints, etc.).  It is used primarily
>    by the function `create_breakpoint'.  */
>
> or some such.
>
>> +struct create_breakpoint_s
>
> <http://sourceware.org/ml/gdb/2012-08/msg00004.html>
>
> I liked Stan's proposal, to name the structure "struct
> breakpoint_properties", so I suggest using this name.
>
>> +{
>> +  /* GDBARCH will be initialized to NULL in
>> +     function CREATE_BREAKPOINT_INIT.  */
>> +  struct gdbarch *gdbarch;
>
> I think this kind of comment is not really necessary.  I would change it
> to something like:
>
> /* The gdbarch associated with the breakpoint.  */
>
> Same for the rest.
>
> [...]
>
>> +  /* Function CREATE_BREAKPOINT has two major modes of operations,
>> +     selected by the PARSE_CONDITION_AND_THREAD parameter.
>> +     If non-zero, the function will parse arg, extracting
>> +     breakpoint location, address and thread.
>> +     Otherwise, ARG is just the location of breakpoint,
>> +     with condition and thread specified by
>> +     the COND_STRING and THREAD parameters.
>> +     It will be initialized to 0 in function CREATE_BREAKPOINT_INIT.  */
>> +  int parse_condition_and_thread;
>
> This comment refers to the `create_breakpoint' function, so it should
> stay there IMO.
>
> Aside of that, I have no other comments.  Thanks again for doing this.
>
> --
> Sergio