From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23239 invoked by alias); 11 Dec 2003 06:00:29 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 23229 invoked from network); 11 Dec 2003 06:00:24 -0000 Received: from unknown (HELO monty-python.gnu.org) (199.232.76.173) by sources.redhat.com with SMTP; 11 Dec 2003 06:00:24 -0000 Received: from [207.232.27.5] (helo=WST0054) by monty-python.gnu.org with asmtp (Exim 4.24) id 1AUKpB-0000nZ-9A; Thu, 11 Dec 2003 02:01:25 -0500 Date: Thu, 11 Dec 2003 06:00:00 -0000 Message-Id: From: Eli Zaretskii To: Jeff Johnston CC: gdb-patches@sources.redhat.com In-reply-to: <3FD7C458.2080404@redhat.com> (message from Jeff Johnston on Wed, 10 Dec 2003 20:11:52 -0500) Subject: Re: [RFA]: breakpoint.c patch (prelude to pending breakpoint support) Reply-to: Eli Zaretskii References: <3FD7C458.2080404@redhat.com> X-SW-Source: 2003-12/txt/msg00310.txt.bz2 > Date: Wed, 10 Dec 2003 20:11:52 -0500 > From: Jeff Johnston > > Ok to commit? I have 2 very minor comments. The first one is about the ChangeLog entries: > 2003-12-10 Jeff Johnston > > * breakpoint.c (breakpoint_enabled): New function to test whether breakpoint is > active and enabled. Is this line really that long, or did your mailer mess it up? If the former, it needs to be reformatted. > ( insert_bp_location, insert_breakpoints): Call new function to test > for enabled breakpoint. > (remove_breakpoint, breakpoint_here_p): Ditto. > (breakpoint_thread_match): Ditto. > (bpstat_should_step, bpstat_have_active_hw_watchpoints): Ditto. > (disable_breakpoints_in_shlibs): Ditto. > (hw_watchpoint_used_count): Ditto. > (disable_watchpoints_before_interactive_call_start): Ditto. > (breakpoint_re_set_one): Ditto. Instead of the long series of "(func): Ditto." kind of entries, it's better to make a single multi-line entry, like this: (remove_breakpoint, breakpoint_here_p, breakpoint_thread_match) (bpstat_should_step, bpstat_have_active_hw_watchpoints) (disable_breakpoints_in_shlibs, hw_watchpoint_used_count) (disable_watchpoints_before_interactive_call_start) (breakpoint_re_set_one): Ditto. (Note how every line ends with a right paren: it's important for Emacs to highlight the function names correctly.) Also, please make sure each line of the ChangeLog entry begins with a literal TAB character. The second comment is about this hunk of changes: > @@ -2574,9 +2581,7 @@ bpstat_stop_status (CORE_ADDR *pc, int n > > ALL_BREAKPOINTS_SAFE (b, temp) > { > - if (b->enable_state == bp_disabled > - || b->enable_state == bp_shlib_disabled > - || b->enable_state == bp_call_disabled) > + if (!breakpoint_enabled (b) && b->enable_state != bp_permanent) > continue; Bother. Is it really wise to replace an explicit check of equality to several bp_* constants with "!= bp_permanent"? Are we sure that any non-bp_permanent breakpoint should pass this test, even if in the future additional bp_* constants will be introduced that aren't there now? Thanks.