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

["J. Johnston" <jjohnstn at redhat dot com>]

Eli Zaretskii wrote:
> > 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.

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.