This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA]: breakpoint.c patch (prelude to pending breakpoint support)
- From: Eli Zaretskii <eliz at elta dot co dot il>
- To: Jeff Johnston <jjohnstn at redhat dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: 11 Dec 2003 08:01:58 +0200
- Subject: Re: [RFA]: breakpoint.c patch (prelude to pending breakpoint support)
- References: <3FD7C458.2080404@redhat.com>
- Reply-to: Eli Zaretskii <eliz at elta dot co dot il>
> Date: Wed, 10 Dec 2003 20:11:52 -0500
> From: Jeff Johnston <jjohnstn@redhat.com>
>
> Ok to commit?
I have 2 very minor comments. The first one is about the ChangeLog
entries:
> 2003-12-10 Jeff Johnston <jjohnstn@redhat.com>
>
> * 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.