From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23411 invoked by alias); 21 Feb 2013 13:58:56 -0000 Received: (qmail 23394 invoked by uid 22791); 21 Feb 2013 13:58:54 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 21 Feb 2013 13:58:36 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1LDwXnl021828 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 21 Feb 2013 08:58:33 -0500 Received: from psique (ovpn-113-23.phx2.redhat.com [10.3.113.23]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r1LDwSnu016618 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 21 Feb 2013 08:58:30 -0500 From: Sergio Durigan Junior To: Hui Zhu Cc: gdb-patches ml , insight , Pedro Alves , Tom Tromey , Stan Shebs , Keith Seitz Subject: Re: [PATCH] change the arguments of create_breakpoint to a struct References: X-URL: http://www.redhat.com Date: Thu, 21 Feb 2013 13:58:00 -0000 In-Reply-To: (Hui Zhu's message of "Thu, 21 Feb 2013 13:39:11 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00026.txt.bz2 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 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