From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16900 invoked by alias); 11 Dec 2003 16:32:48 -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 16885 invoked from network); 11 Dec 2003 16:32:47 -0000 Received: from unknown (HELO touchme.toronto.redhat.com) (216.129.200.20) by sources.redhat.com with SMTP; 11 Dec 2003 16:32:47 -0000 Received: from redhat.com (toocool.toronto.redhat.com [172.16.14.72]) by touchme.toronto.redhat.com (Postfix) with ESMTP id ED5B280018E; Thu, 11 Dec 2003 11:32:46 -0500 (EST) Message-ID: <3FD89C2E.9070906@redhat.com> Date: Thu, 11 Dec 2003 16:32:00 -0000 From: "J. Johnston" Organization: Red Hat Inc. User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Eli Zaretskii Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA]: breakpoint.c patch (prelude to pending breakpoint support) References: <3FD7C458.2080404@redhat.com> In-Reply-To: Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2003-12/txt/msg00321.txt.bz2 Eli Zaretskii wrote: >>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. > Eli, I realize you are just making a minor comment, but can I ask that gdb maintainers please start trusting me on this. My ChangeLog entries are just typed into my note (i.e. I do not cut and paste from the actual ChangeLog). I "always" retype the ChangeLog entry when and if the patch is accepted so the line length and white-spacing you see in the note is completely moot. If anybody is unhappy with my previous ChangeLog entries, feel free to let me know. >> ( 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. > Ok, will do. > (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? > No I can't predict possible future enable states. However, the change was suggested by Daniel and he is much closer to the code than I am. I would think that whatever new value was added, all tests of the enable_state would have to be analyzed and dealt with; this one included. I have no problems with changing it to back to a simple test if people are uncomfortable with it. -- Jeff J.