From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20418 invoked by alias); 22 Feb 2011 09:17:03 -0000 Received: (qmail 20366 invoked by uid 22791); 22 Feb 2011 09:17:01 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 22 Feb 2011 09:16:56 +0000 Received: (qmail 31656 invoked from network); 22 Feb 2011 09:16:54 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Feb 2011 09:16:54 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] info break/watch/trace use get_number_or_range, take two Date: Tue, 22 Feb 2011 09:27:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.35-25-generic; KDE/4.6.0; x86_64; ; ) Cc: Michael Snyder References: <4D62E5E0.9080105@vmware.com> In-Reply-To: <4D62E5E0.9080105@vmware.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201102220916.51541.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-02/txt/msg00575.txt.bz2 On Monday 21 February 2011 22:23:28, Michael Snyder wrote: > Supercedes previous submission. > > Fixed Pedro's issues. Tested with maint info break. "Pedro's issues"? Eh, I have no personal issues! Thanks, I like this much better. > + /* If we have an "args" string, it is a list of breakpoints to > + accept. Skip the others. */ > + if (args != NULL && *args != '\0') > + { > + if (allflag && (parse_and_eval_long (args) != b->number)) Spurious parens? > + continue; > + if (!allflag && !number_is_in_list (args, b->number)) > + continue; > + } I think that parse_and_eval_long means that "maint info break 2-3" will print breakpoint -1, while "info break 2-3" will print breakpoints 2 and 3 It's fine with me, just pointing it out. > - addr_bit = breakpoint_address_bits (b); > - if (addr_bit > print_address_bits) > - print_address_bits = addr_bit; > + if (allflag || user_breakpoint_p (b)) > + { > + int addr_bit, type_len; > > - type_len = strlen (bptype_string (b->type)); > - if (type_len > print_type_col_width) > - print_type_col_width = type_len; > + addr_bit = breakpoint_address_bits (b); > + if (addr_bit > print_address_bits) > + print_address_bits = addr_bit; > > - nr_printable_breakpoints++; > - } > - } > + type_len = strlen (bptype_string (b->type)); > + if (type_len > print_type_col_width) > + print_type_col_width = type_len; > + > + nr_printable_breakpoints++; > + } > + } > > if (opts.addressprint) > bkpttbl_chain > @@ -5169,16 +5178,16 @@ breakpoint_1 (int bnum, int allflag, > annotate_field (3); > ui_out_table_header (uiout, 3, ui_left, "enabled", "Enb"); /* 4 */ > if (opts.addressprint) > - { > - if (nr_printable_breakpoints > 0) > - annotate_field (4); > - if (print_address_bits <= 32) > - ui_out_table_header (uiout, 10, ui_left, > - "addr", "Address"); /* 5 */ > - else > - ui_out_table_header (uiout, 18, ui_left, > - "addr", "Address"); /* 5 */ > - } > + { > + if (nr_printable_breakpoints > 0) > + annotate_field (4); > + if (print_address_bits <= 32) > + ui_out_table_header (uiout, 10, ui_left, > + "addr", "Address"); /* 5 */ > + else > + ui_out_table_header (uiout, 18, ui_left, > + "addr", "Address"); /* 5 */ > + } Please let's keep big formatting changes separate from functional changes. We ask that in patch submissions, so we should follow the same practice ourselves. It's really a good rule BTW. I think of that as one case of what I call it The Principle of Least Reversibility (TM), so explained: If by chance this patch needs to be reverted, you don't want to revert the whitespace fixes. Hence, you'll want to make the whitespace fixes a separate patch. The same rule applies to preparatory fixes and changes, and code moves (where if you move the code, you should NOT change it in any other way). > add_info ("breakpoints", breakpoints_info, _("\ > -Status of user-settable breakpoints, or breakpoint number NUMBER.\n\ > +Status of user-settable breakpoints listed, or all breakpoints if no argument.\n\ "listed" doesn't sound obviously referring to the spec you pass as argument to the command. "listed where? the command itself is printing a list." was my thought. Is there any other way to spell that? -- Pedro Alves