Re: [PATCH] Hooks still needed for annotations

[Daniel Jacobowitz <drow at false dot org>]

On Sat, Jun 04, 2005 at 03:18:55PM +1200, Nick Roberts wrote:
>  > The way to prevent them from being removed again is to add test cases.
> 
> OK, I've done this. Running the tests revealed a small bug with
>  "set annotate" -- it didn't call _initialize_annotate to set the hooks.
> This must be why the tests didn't fail when the hooks were removed in the
> first place.
> 
> I've only tested for breakpoints-invalid annotations when breakpoints
> have been deleted as I could simply adapt existing tests. They should
> also be present when breakpoints are enabled/disabled. However, this test
> should be sufficient to prevent the inadvertant removal of the hooks.

OK, having answered the rest of the side topics in this discussion I
now come to the actual patch...

Having reread the discussion I would like to ask you what your goal is
with this patch.  You don't use this annotation, and you've said that
it is very awkward to use because of the amount of output it produces.
Why should we fix it (as opposed to garbage collecting it) if no one
has missed it?

About the patch:

> *** /home/nick/src/gdb/event-top.c.~1.40.~	2005-03-17 12:23:51.000000000 +1300
> --- /home/nick/src/gdb/event-top.c	2005-06-04 14:36:37.000000000 +1200
> ***************
> *** 43,48 ****
> --- 43,50 ----
>   /* readline defines this.  */
>   #undef savestring
>   
> + extern void _initialize_annotate (void);
> + 
>   static void rl_callback_read_char_wrapper (gdb_client_data client_data);
>   static void command_line_handler (char *rl);
>   static void command_line_handler_continuation (struct continuation_arg *arg);

No extern prototypes.  If you need to call a function from another
file, it should be defined in a header, and the header should be
included in both the defining and using files.  That's the only way in
C we can make sure that argument mismatch does not creep in.  Also,
please move this into a new function if you want it to be called other
than from init.c; the _initialize_* functions are magic.

> *** /home/nick/src/gdb/interps.c.~1.15.~	2005-04-26 21:44:15.000000000 +1200
> --- /home/nick/src/gdb/interps.c	2005-06-04 15:12:16.000000000 +1200
> ***************
> *** 326,333 ****
>     deprecated_query_hook = 0;
>     deprecated_warning_hook = 0;
>     deprecated_create_breakpoint_hook = 0;
> -   deprecated_delete_breakpoint_hook = 0;
> -   deprecated_modify_breakpoint_hook = 0;
>     deprecated_interactive_hook = 0;
>     deprecated_registers_changed_hook = 0;
>     deprecated_readline_begin_hook = 0;
> --- 326,331 ----

deprecated_create_breakpoint_hook is now set nowhere.

deprecated_delete_breakpoint_hook and deprecated_modify_breakpoint_hook
are set only by annotations.  There's a total of three call sites
between the two hooks.  And each of those takes up more screen real
estate than a call to breakpoints_changed, which is what they
eventually become.

The comments in mention() suggest that at one time, GDB was trying to
move away from breakpoints_changed to a more specific set of hooks. 
But now the hooks are more or less dead, and to get full mileage out of
them they're going to need a redesign.  So maybe we should just delete
all three hooks, and replace the two that annotations use with
calls to breakpoints_changed.


-- 
Daniel Jacobowitz
CodeSourcery, LLC