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

["J. Johnston" <jjohnstn@redhat.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.