Re: [RFA] Refactor breakpoint_re_set_one

[Joel Brobecker <brobecker at adacore dot com>]

> 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