This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [4/9] associate bpstat with location
- From: Eli Zaretskii <eliz at gnu dot org>
- To: Vladimir Prus <vladimir at codesourcery dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Sat, 08 Sep 2007 14:07:08 +0300
- Subject: Re: [4/9] associate bpstat with location
- References: <200709080018.25052.vladimir@codesourcery.com>
- Reply-to: Eli Zaretskii <eliz at gnu dot org>
> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 8 Sep 2007 00:18:24 +0400
>
> (bpstat_find_breakpoint): Look at bpstat's location's
> owner, not at bpstat->breakpoint_at.
> (bpstat_find_step_resume_breakpoint): Likewise.
> (bpstat_num): Likewise.
> (print_it_typical): Likewise.
> (print_bp_stop_message): Likewise.
> (watchpoint_check): Likewise.
> (bpstat_what): Likewise.
> (bpstat_get_triggered_catchpoints): Likewise.
> (breakpoint_auto_delete): Likewise.
> (delete_breakpoint): Likewise.
A minor stylistic point: could we please avoid the annoying
"Likewise"s? The canonical way of writing a ChangeLog entry for
several functions with an identical change is this:
(bpstat_find_breakpoint, bpstat_find_step_resume_breakpoint)
(bpstat_num, print_it_typical): Look at bpstat's location's
owner, not at bpstat->breakpoint_at.
I'm quite sure the GNU coding standards describe this. (Yes, I know
that our ChangeLog's abuse "Likewise" too much.)
> - b = (*bsp)->breakpoint_at;
> + /* We assume we'll never have several bpstats that
> + correspond to a single breakpoint -- otherwise,
> + this function might return the same number more
> + than once and this will look ugly. */
> + b = (*bsp)->breakpoint_at ? (*bsp)->breakpoint_at->owner : NULL;
Is the assumption in the comment above really valid? I happen to put
several breakpoints on the same line quite a lot (each breakpoint has
a different condition and/or different commands list).
Can we do better, even if it requires to try harder?
> case bp_access_watchpoint:
> if (bs->old_val != NULL)
> {
> - annotate_watchpoint (bs->breakpoint_at->number);
> + annotate_watchpoint (b->number);
Watchpoints also? Did you make corresponding changes in the code that
sets watchpoints?
Thanks.