Re: [RFA]: breakpoint.c patch (prelude to pending breakpoint support)

[Eli Zaretskii <eliz@elta.co.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.