This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Refactor breakpoint_re_set_one
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Mon, 28 Mar 2011 11:46:47 -0700
- Subject: Re: [RFA] Refactor breakpoint_re_set_one
- References: <1301322874.2433.0.camel@hactar>
> 2011-03-27 Thiago Jung Bauermann <bauerman@br.ibm.com>
>
> * breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting
> code from here ...
> (reset_breakpoint): ... to here ...
> (addr_string_to_sals): ... and here.
I didn't really verify line by line that you just extracted out
the code without actually changing something, but I think we can
trust your copy/paste tool :-).
No real issue on my end of things (just a few little nits).
But I'm no longer a specialist of this area, so I'd wait a little
to see if anyone else might have some comments (say, until the
end of the week?).
My comments below:
> +/* Find the SaL locations corresponding to the given addr_string. */
By convention `addr_string' should be capitalized. It's one of these
things I really wonder why we do it, but anyways...
> + /* For pending breakpoints, it's expected that parsing will
> + fail until the right shared library is loaded. User has
> + already told to create pending breakpoints and don't need
^^^^ doesn't
> + extra messages. If breakpoint is in bp_shlib_disabled
> + state, then user already saw the message about that
> + breakpoint being disabled, and don't want to see more
^^^^^ doesn't
> + errors. */
> +/* Reset a hardware or software breakpoint. */
> +
> +static void
> +reset_breakpoint (struct breakpoint *b)
My only comment is that `reset' is a little ambiguous, but maybe
that's just me. I often think of "reset" as set back to original
value. I like the use of `re_set' in breakpoint_re_set_one, so
what do you think of doing the same here? Also, a more descriptive
description of the function would be useful, IMO.
--
Joel