[RFA]: breakpoint.c patch (prelude to pending breakpoint support)
J. Johnston
jjohnstn@redhat.com
Thu Dec 11 16:32:00 GMT 2003
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.
More information about the Gdb-patches
mailing list