From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9058 invoked by alias); 22 Feb 2011 18:31:10 -0000 Received: (qmail 9050 invoked by uid 22791); 22 Feb 2011 18:31:09 -0000 X-SWARE-Spam-Status: No, hits=-4.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 22 Feb 2011 18:31:04 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id CF5F155004; Tue, 22 Feb 2011 10:31:02 -0800 (PST) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost4.vmware.com (Postfix) with ESMTP id C6C18C9D28; Tue, 22 Feb 2011 10:31:02 -0800 (PST) Message-ID: <4D6400E6.6000303@vmware.com> Date: Tue, 22 Feb 2011 18:31:00 -0000 From: Michael Snyder User-Agent: Thunderbird 2.0.0.24 (X11/20101201) MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" Subject: Re: [RFA] info break/watch/trace use get_number_or_range, take two References: <4D62E5E0.9080105@vmware.com> <201102220916.51541.pedro@codesourcery.com> In-Reply-To: <201102220916.51541.pedro@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit 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/msg00608.txt.bz2 Pedro Alves wrote: > 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? OK >> + 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. Just preserving the existing behavior. >> - 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? It's getting hard to describe. Do you have a suggestion?