This is the mail archive of the
insight@sourceware.org
mailing list for the Insight project.
Re: [PATCH] change the arguments of create_breakpoint to a struct
- From: Hui Zhu <teawater at gmail dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>, insight <insight at sourceware dot org>, Pedro Alves <palves at redhat dot com>, Tom Tromey <tromey at redhat dot com>, Stan Shebs <stan_shebs at mentor dot com>, Keith Seitz <keiths at redhat dot com>
- Date: Fri, 22 Feb 2013 10:14:23 +0800
- Subject: Re: [PATCH] change the arguments of create_breakpoint to a struct
- References: <CANFwon3scDzj1yJaMzbEGpeMx1w4Ton9gpuh9X3HfBF3xmppDA@mail.gmail.com> <m3bobdg458.fsf@redhat.com>
Hi Sergio,
Thanks for your review.
On Thu, Feb 21, 2013 at 9:58 PM, Sergio Durigan Junior
<sergiodj@redhat.com> 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