Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops

Jan Kratochvil
2010-11-16T04:06:49Z

On Wed, 20 Oct 2010 02:31:31 +0200, Thiago Jung Bauermann wrote:
> -static void
> -insert_catch_fork (struct breakpoint *b)
> +static int
> +insert_catch_fork (struct bp_location *b)

Such variables (across the whole patch) should be really renamed when changing
its type.

I understand there are already cases of such incorrect naming in GDB but we
should not make it worse.

Also were these functions intended per-breakpoint or per-bp_location?
It looks to me currently they are used only for single-location breakpoint so
no one knows. (I guess they were meant for breakpoint.)

> -struct breakpoint_ops
> +struct breakpoint_ops
> {
> - /* Insert the breakpoint or activate the catchpoint. Should raise
> - an exception if the operation failed. */
> - void (*insert) (struct breakpoint *);
> + /* Insert the breakpoint or watchpoint or activate the catchpoint.
> + Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
> + type is not supported, -1 for failure. */
> + int (*insert) (struct bp_location *);
>
> /* Remove the breakpoint/catchpoint that was previously inserted
> - with the "insert" method above. Return non-zero if the operation
> - succeeded. */
> - int (*remove) (struct breakpoint *);
> + with the "insert" method above. Return 0 for success, 1 if the
> + breakpoint, watchpoint or catchpoint type is not supported,
> + -1 for failure. */
> + int (*remove) (struct bp_location *);

At least rename it to insert_bploc (or insert_location etc.). This will need
to be cleaned up with the regular breakpoints/watchpoints conversion to
breakpoint_ops.

Thanks,
Jan