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

[Joel Brobecker <brobecker@adacore.com>]

> 2010-10-19  Thiago Jung Bauermann  <bauerman@br.ibm.com>
> 
> 	Convert hardware watchpoints to use breakpoint_ops.
> 
> gdb/
> 	* breakpoint.h (breakpoint_ops) <insert>: Return int instead of void.
> 	Accept pointer to struct bp_location instead of pointer to
> 	struct breakpoint.  Adapt all implementations.
> 	(breakpoint_ops) <remove>: Accept pointer to struct bp_location instead
> 	of pointer to struct breakpoint.  Adapt all implementations.
> 	* breakpoint.c (insert_catchpoint): Delete function.
> 	(insert_bp_location): Call the watchpoint or catchpoint's
> 	breakpoint_ops.insert method.
> 	(remove_breakpoint_1): Call the watchpoint or catchpoint's
> 	breakpoint_ops.remove method.
> 	(insert_watchpoint, remove_watchpoint): New functions.
> 	(watchpoint_breakpoint_ops): New structure.
> 	(watch_command_1): Initialize the OPS field.
> 	* inf-child.c (inf_child_insert_fork_catchpoint)
> 	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
> 	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
> 	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
> 	Delete functions.
> 	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
> 	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
> 	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
> 	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
> 	* target.c (update_current_target): Change default implementation of
> 	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
> 	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
> 	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
> 	to_set_syscall_catchpoint to return_one.
> 	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
> 	(debug_to_insert_exec_catchpoint): Report return value.
> 	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
> 	(to_insert_fork_catchpoint): Change declaration to return int instead
> 	of void.
> 
> gdb/testsuite/
> 	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
> 	type is not supported.
> 	* gdb.base/foll-fork.exp: Likewise.
> 	* gdb.base/foll-vfork.exp: Likewise.

I'm OK with this patch.  Just a possible suggestion below...
Can you wait a couple more days to give anyone one last chance
at making comments on this patch? After that, please go ahead
and commit.

> +	  if (val == 1)
> +	    warning (_("\
> +Inserting catchpoint %d: Your system does not support this type of catchpoint."),
> +		     bpt->owner->number);
> +	  else
> +	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);

Just curious: Why not say "Error inserting catchpoint %d" in both cases
(we would still keep the ": <not supported>" part in the first case)?

> -    void (*to_insert_fork_catchpoint) (int);
> +    int (*to_insert_fork_catchpoint) (int);
>      int (*to_remove_fork_catchpoint) (int);
> -    void (*to_insert_vfork_catchpoint) (int);
> +    int (*to_insert_vfork_catchpoint) (int);
>      int (*to_remove_vfork_catchpoint) (int);
>      int (*to_follow_fork) (struct target_ops *, int);
> -    void (*to_insert_exec_catchpoint) (int);
> +    int (*to_insert_exec_catchpoint) (int);
>      int (*to_remove_exec_catchpoint) (int);
>      int (*to_set_syscall_catchpoint) (int, int, int, int, int *);

I think we really should be documenting at least the return value.
Apparently, at least some of these "method" are documented though
their associated "target_<...>" macro/function.  I'd rather see
that documentation next to the method rather than the macro, but
that's for another discussion.


-- 
Joel