This is the mail archive of the insight@sourceware.org mailing list for the Insight project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

>  /* 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]