From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30599 invoked by alias); 22 Feb 2013 02:15:15 -0000 Received: (qmail 30483 invoked by uid 22791); 22 Feb 2013 02:15:13 -0000 X-SWARE-Spam-Status: No, hits=-4.8 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-oa0-f52.google.com (HELO mail-oa0-f52.google.com) (209.85.219.52) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 22 Feb 2013 02:15:04 +0000 Received: by mail-oa0-f52.google.com with SMTP id k14so175840oag.11 for ; Thu, 21 Feb 2013 18:15:04 -0800 (PST) X-Received: by 10.60.22.233 with SMTP id h9mr110290oef.47.1361499304125; Thu, 21 Feb 2013 18:15:04 -0800 (PST) MIME-Version: 1.0 Received: by 10.60.46.133 with HTTP; Thu, 21 Feb 2013 18:14:23 -0800 (PST) In-Reply-To: References: From: Hui Zhu Date: Fri, 22 Feb 2013 02:15:00 -0000 Message-ID: Subject: Re: [PATCH] change the arguments of create_breakpoint to a struct To: Sergio Durigan Junior Cc: gdb-patches ml , insight , Pedro Alves , Tom Tromey , Stan Shebs , Keith Seitz Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes Mailing-List: contact insight-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: insight-owner@sourceware.org X-SW-Source: 2013-q1/txt/msg00027.txt.bz2 Hi Sergio, Thanks for your review. On Thu, Feb 21, 2013 at 9:58 PM, Sergio Durigan Junior 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 > > > > 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