public inbox for insight@sourceware.org
 help / color / mirror / Atom feed
From: Hui Zhu <teawater@gmail.com>
To: Sergio Durigan Junior <sergiodj@redhat.com>
Cc: gdb-patches ml <gdb-patches@sourceware.org>,
	insight <insight@sourceware.org>,
		Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>,
		Stan Shebs <stan_shebs@mentor.com>,
	Keith Seitz <keiths@redhat.com>
Subject: Re: [PATCH] change the arguments of create_breakpoint to a struct
Date: Fri, 22 Feb 2013 02:15:00 -0000	[thread overview]
Message-ID: <CANFwon2F-DoTnf9=wEctzEbVnURSVjrFjeCjMFhf9bDmFYdwFQ@mail.gmail.com> (raw)
In-Reply-To: <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

      reply	other threads:[~2013-02-22  2:15 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-21  5:40 Hui Zhu
2013-02-21 13:58 ` Sergio Durigan Junior
2013-02-22  2:15   ` Hui Zhu [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANFwon2F-DoTnf9=wEctzEbVnURSVjrFjeCjMFhf9bDmFYdwFQ@mail.gmail.com' \
    --to=teawater@gmail.com \
    --cc=gdb-patches@sourceware.org \
    --cc=insight@sourceware.org \
    --cc=keiths@redhat.com \
    --cc=palves@redhat.com \
    --cc=sergiodj@redhat.com \
    --cc=stan_shebs@mentor.com \
    --cc=tromey@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).