RFA: Patch: annotations -vs- deprecated hooks

RFA: Patch: annotations -vs- deprecated hooks

[Tom Tromey]

This patch removes a few deprecated hooks associated with annotations.

This requires my previous observer patch.

Some of these are only used by annotations; I changed the annotation
code to use observers instead.  Some of these hooks are called by
annotations but never set.  These I just deleted.

There are still a couple of hooks called by the annotations code that
are used by gdbtk.  This setup seems weird to me.  I guess these ought
to be replaced by observers.

Built and regression tested on x86 F8.  Ok?

Tom

b/gdb/ChangeLog:
2008-07-13  Tom Tromey  <tromey@redhat.com>

	* annotate.h (deprecated_annotate_starting_hook): Remove.
	(deprecated_annotate_stopped_hook): Remove.
	(deprecated_annotate_exited_hook): Remove.
	* Makefile.in (annotate.o): Depend on observer_h.
	* top.c (deprecated_delete_breakpoint_hook): Remove.
	(deprecated_create_breakpoint_hook): Likewise.
	(deprecated_modify_breakpoint_hook): Likewise.
	* interps.c (clear_interpreter_hooks): Update for removed hooks.
	* breakpoint.c (mention): Don't call removed hook.
	(delete_breakpoint): Likewise.
	(disable_breakpoint): Likewise.
	(do_enable_breakpoint): Likewise.
	* annotate.c: Include observer.h.
	(breakpoint_changed): Change type of argument.
	(_initialize_annotate): Register observers.
	(deprecated_annotate_starting_hook): Remove.
	(deprecated_annotate_stopped_hook): Remove.
	(deprecated_annotate_exited_hook): Remove.
	(annotate_starting): Update for hook removal.
	(annotate_stopped): Likewise.
	(annotate_exited): Likewise.
	* defs.h (deprecated_delete_breakpoint_hook): Remove.
	(deprecated_create_breakpoint_hook): Likewise.
	(deprecated_modify_breakpoint_hook): Likewise.

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 4661d7b..1f1b81b 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1908,7 +1908,7 @@ amd64-tdep.o: amd64-tdep.c $(defs_h) $(arch_utils_h) $(block_h) \
 	$(regset_h) $(symfile_h) $(gdb_assert_h) $(amd64_tdep_h) \
 	$(i387_tdep_h)
 annotate.o: annotate.c $(defs_h) $(annotate_h) $(value_h) $(target_h) \
-	$(gdbtypes_h) $(breakpoint_h)
+	$(gdbtypes_h) $(breakpoint_h) $(observer_h)
 arch-utils.o: arch-utils.c $(defs_h) $(arch_utils_h) $(buildsym_h) \
 	$(gdbcmd_h) $(inferior_h) $(gdb_string_h) $(regcache_h) \
 	$(gdb_assert_h) $(sim_regno_h) $(gdbcore_h) $(osabi_h) $(version_h) \
diff --git a/gdb/annotate.c b/gdb/annotate.c
index 98a37ae..ed608b6 100644
--- a/gdb/annotate.c
+++ b/gdb/annotate.c
@@ -23,6 +23,7 @@
 #include "target.h"
 #include "gdbtypes.h"
 #include "breakpoint.h"
+#include "observer.h"
 \f
 
 /* Prototypes for local functions. */
@@ -31,13 +32,11 @@ extern void _initialize_annotate (void);
 
 static void print_value_flags (struct type *);
 
-static void breakpoint_changed (struct breakpoint *);
+static void breakpoint_changed (int);
+
 
-void (*deprecated_annotate_starting_hook) (void);
-void (*deprecated_annotate_stopped_hook) (void);
 void (*deprecated_annotate_signalled_hook) (void);
 void (*deprecated_annotate_signal_hook) (void);
-void (*deprecated_annotate_exited_hook) (void);
 
 static int ignore_count_changed = 0;
 
@@ -99,28 +98,15 @@ annotate_watchpoint (int num)
 void
 annotate_starting (void)
 {
-
-  if (deprecated_annotate_starting_hook)
-    deprecated_annotate_starting_hook ();
-  else
-    {
-      if (annotation_level > 1)
-	{
-	  printf_filtered (("\n\032\032starting\n"));
-	}
-    }
+  if (annotation_level > 1)
+    printf_filtered (("\n\032\032starting\n"));
 }
 
 void
 annotate_stopped (void)
 {
-  if (deprecated_annotate_stopped_hook)
-    deprecated_annotate_stopped_hook ();
-  else
-    {
-      if (annotation_level > 1)
-	printf_filtered (("\n\032\032stopped\n"));
-    }
+  if (annotation_level > 1)
+    printf_filtered (("\n\032\032stopped\n"));
   if (annotation_level > 1 && ignore_count_changed)
     {
       ignore_count_changed = 0;
@@ -131,13 +117,8 @@ annotate_stopped (void)
 void
 annotate_exited (int exitstatus)
 {
-  if (deprecated_annotate_exited_hook)
-    deprecated_annotate_exited_hook ();
-  else
-    {
-      if (annotation_level > 1)
-	printf_filtered (("\n\032\032exited %d\n"), exitstatus);
-    }
+  if (annotation_level > 1)
+    printf_filtered (("\n\032\032exited %d\n"), exitstatus);
 }
 
 void
@@ -578,7 +559,7 @@ annotate_array_section_end (void)
 }
 
 static void
-breakpoint_changed (struct breakpoint *b)
+breakpoint_changed (int bpno)
 {
   breakpoints_changed ();
 }
@@ -588,7 +569,7 @@ _initialize_annotate (void)
 {
   if (annotation_level == 2)
     {
-      deprecated_delete_breakpoint_hook = breakpoint_changed;
-      deprecated_modify_breakpoint_hook = breakpoint_changed;
+      observer_attach_breakpoint_deleted (breakpoint_changed);
+      observer_attach_breakpoint_modified (breakpoint_changed);
     }
 }
diff --git a/gdb/annotate.h b/gdb/annotate.h
index bbc274f..6fb1954 100644
--- a/gdb/annotate.h
+++ b/gdb/annotate.h
@@ -99,8 +99,5 @@ extern void annotate_elt_rep_end (void);
 extern void annotate_elt (void);
 extern void annotate_array_section_end (void);
 
-extern void (*deprecated_annotate_starting_hook) (void);
-extern void (*deprecated_annotate_stopped_hook) (void);
 extern void (*deprecated_annotate_signalled_hook) (void);
 extern void (*deprecated_annotate_signal_hook) (void);
-extern void (*deprecated_annotate_exited_hook) (void);
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4f4f78f..4fde5a5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4889,10 +4889,7 @@ mention (struct breakpoint *b)
   /* FIXME: This is misplaced; mention() is called by things (like
      hitting a watchpoint) other than breakpoint creation.  It should
      be possible to clean this up and at the same time replace the
-     random calls to breakpoint_changed with this hook, as has already
-     been done for deprecated_delete_breakpoint_hook and so on.  */
-  if (deprecated_create_breakpoint_hook)
-    deprecated_create_breakpoint_hook (b);
+     random calls to breakpoint_changed with this hook.  */
   observer_notify_breakpoint_created (b->number);
 
   if (b->ops != NULL && b->ops->print_mention != NULL)
@@ -7212,8 +7209,6 @@ delete_breakpoint (struct breakpoint *bpt)
   if (bpt->type == bp_none)
     return;
 
-  if (deprecated_delete_breakpoint_hook)
-    deprecated_delete_breakpoint_hook (bpt);
   observer_notify_breakpoint_deleted (bpt->number);
 
   if (breakpoint_chain == bpt)
@@ -7867,8 +7862,6 @@ disable_breakpoint (struct breakpoint *bpt)
 
   update_global_location_list (0);
 
-  if (deprecated_modify_breakpoint_hook)
-    deprecated_modify_breakpoint_hook (bpt);
   observer_notify_breakpoint_modified (bpt->number);
 }
 
@@ -7992,8 +7985,6 @@ have been allocated for other watchpoints.\n"), bpt->number);
   update_global_location_list (1);
   breakpoints_changed ();
   
-  if (deprecated_modify_breakpoint_hook)
-    deprecated_modify_breakpoint_hook (bpt);
   observer_notify_breakpoint_modified (bpt->number);
 }
 
diff --git a/gdb/defs.h b/gdb/defs.h
index 6e6aa80..cae619e 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -1053,9 +1053,6 @@ extern int (*deprecated_query_hook) (const char *, va_list)
 extern void (*deprecated_warning_hook) (const char *, va_list)
      ATTRIBUTE_FPTR_PRINTF(1,0);
 extern void (*deprecated_flush_hook) (struct ui_file * stream);
-extern void (*deprecated_create_breakpoint_hook) (struct breakpoint * b);
-extern void (*deprecated_delete_breakpoint_hook) (struct breakpoint * bpt);
-extern void (*deprecated_modify_breakpoint_hook) (struct breakpoint * bpt);
 extern void (*deprecated_interactive_hook) (void);
 extern void (*deprecated_readline_begin_hook) (char *, ...)
      ATTRIBUTE_FPTR_PRINTF_1;
diff --git a/gdb/interps.c b/gdb/interps.c
index 194ac62..3bb7811 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -336,9 +336,6 @@ clear_interpreter_hooks (void)
   /*print_frame_more_info_hook = 0; */
   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_readline_begin_hook = 0;
   deprecated_readline_hook = 0;
diff --git a/gdb/top.c b/gdb/top.c
index 7ef539f..7f46caf 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -246,13 +246,6 @@ void (*deprecated_readline_begin_hook) (char *, ...);
 char *(*deprecated_readline_hook) (char *);
 void (*deprecated_readline_end_hook) (void);
 
-/* Called as appropriate to notify the interface of the specified breakpoint
-   conditions.  */
-
-void (*deprecated_create_breakpoint_hook) (struct breakpoint * bpt);
-void (*deprecated_delete_breakpoint_hook) (struct breakpoint * bpt);
-void (*deprecated_modify_breakpoint_hook) (struct breakpoint * bpt);
-
 /* Called as appropriate to notify the interface that we have attached
    to or detached from an already running process. */
 

Re: RFA: Patch: annotations -vs- deprecated hooks

[Nick Roberts]

 > This patch removes a few deprecated hooks associated with annotations.
 > 
 > This requires my previous observer patch.
 > 
 > Some of these are only used by annotations; I changed the annotation
 > code to use observers instead.  Some of these hooks are called by
 > annotations but never set.  These I just deleted.
 > 
 > There are still a couple of hooks called by the annotations code that
 > are used by gdbtk.  This setup seems weird to me.  I guess these ought
 > to be replaced by observers.
 > 
 > Built and regression tested on x86 F8.  Ok?

The plan is to remove annotations wnen GDB/MI has full functionality anyway.
Is there much benefit from doing this?  If not, I would suggest that "if it
ain't broke don't fix it".

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: RFA: Patch: annotations -vs- deprecated hooks

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

Nick> The plan is to remove annotations wnen GDB/MI has full
Nick> functionality anyway.  Is there much benefit from doing this?
Nick> If not, I would suggest that "if it ain't broke don't fix it".

After being confused by the event/hook/observer stuff while working on
Python, I decided that I'd try to get rid of the deprecated event
mechanisms.  It never occurred to me that gdb maintainers might not
want this, so I didn't ask about it first.

Is it just annotations that you care about, or all the deprecated hook
stuff?  I think it all falls into the category of not-broken.

Tom

Re: RFA: Patch: annotations -vs- deprecated hooks

[Nick Roberts]

Tom Tromey writes:
 > >>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:
 > 
 > Nick> The plan is to remove annotations wnen GDB/MI has full
 > Nick> functionality anyway.  Is there much benefit from doing this?
 > Nick> If not, I would suggest that "if it ain't broke don't fix it".
 > 
 > After being confused by the event/hook/observer stuff while working on
 > Python, I decided that I'd try to get rid of the deprecated event
 > mechanisms.  It never occurred to me that gdb maintainers might not
 > want this, so I didn't ask about it first.

I'm not a maintainer, but maybe that's not what you meant.

 > Is it just annotations that you care about, or all the deprecated hook
 > stuff?  I think it all falls into the category of not-broken.

Emacs still uses annotations and it seems to work OK.  Your earlier change,
emoving event hooks, removes a significant amount of code and stops others from
using them in the future.  This patch, however, while probably safe (I've not
checked) doesn't seem worthwhile to me since this code _will_ disappear in the
future anyway.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: RFA: Patch: annotations -vs- deprecated hooks

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

>> Is it just annotations that you care about, or all the deprecated hook
>> stuff?  I think it all falls into the category of not-broken.

Nick> Emacs still uses annotations and it seems to work OK.  Your
Nick> earlier change, emoving event hooks, removes a significant
Nick> amount of code and stops others from using them in the future.

I think this change is worthwhile as a cleanup.  It removes some
more deprecated things, preventing use of them.

When browsing gdb, I find it very strange to see how many things are
marked deprecated, and for how long.  E.g.,
deprecated_annotate_starting_hook is not set anywhere in gdb, but has
been deprecated since 2004.  That seems like an inordinately long
deprecation for something which is unused.

Nick> This patch, however, while probably safe (I've not checked)
Nick> doesn't seem worthwhile to me since this code _will_ disappear
Nick> in the future anyway.

Do you know when this will be?  If it is soon, then I don't mind
holding off.  If it will be a year, or years, then I think it would be
strange to reject a cleanup in favor of some distant, unwritten patch.

Tom

Re: RFA: Patch: annotations -vs- deprecated hooks

[Daniel Jacobowitz]

On Sun, Jul 13, 2008 at 06:49:43PM -0600, Tom Tromey wrote:
> Nick> This patch, however, while probably safe (I've not checked)
> Nick> doesn't seem worthwhile to me since this code _will_ disappear
> Nick> in the future anyway.
> 
> Do you know when this will be?  If it is soon, then I don't mind
> holding off.  If it will be a year, or years, then I think it would be
> strange to reject a cleanup in favor of some distant, unwritten patch.

In practice I don't think it will ever happen.  There are inactive or
barely active projects which make use of the annotations and as long
as they generally get along with newer GDBs I don't see a need to
remove them - as long as the implementation is not otherwise a
problem.

We do still need to discourage new uses, though.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: Patch: annotations -vs- deprecated hooks

[Nick Roberts]

 > When browsing gdb, I find it very strange to see how many things are
 > marked deprecated, and for how long.  E.g.,
 > deprecated_annotate_starting_hook is not set anywhere in gdb, but has
 > been deprecated since 2004.  That seems like an inordinately long
 > deprecation for something which is unused.

In Emacs, variables get marked as obsolete but they are rarely removed from the
code.  I think that RMS' view is that marking them as obsolete simply
discourages _further_ use of them and that legacy software requires that they
stay.

 > Nick> This patch, however, while probably safe (I've not checked)
 > Nick> doesn't seem worthwhile to me since this code _will_ disappear
 > Nick> in the future anyway.
 > 
 > Do you know when this will be?  If it is soon, then I don't mind
 > holding off.  If it will be a year, or years, then I think it would be
 > strange to reject a cleanup in favor of some distant, unwritten patch.

I could tell you when Emacs no longer needs them, but even then I guess
people will still be using old copies, and then there may be other
projects.  So it probably is a long way off.

Creating observers just for breakpoints results in a mixed approach for
annotations as others don't use observers and removing the deprecated label
might encourage their use.  Also, in the past, I have submitted a patch for
similar observers for use with MI which could confuse.  

  http://sourceware.org/ml/gdb-patches/2008-06/msg00000.html
  http://sourceware.org/ml/gdb-patches/2008-06/msg00012.html
  http://sourceware.org/ml/gdb-patches/2008-06/msg00018.html

I would rather these were incorporated first since MI is the future.

I don't feel that strongly about it though and if the changes are approved
well before a release I can test them against Emacs.  Certainly (all) the
the deprecated_annotate_*_hooks could go.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: RFA: Patch: annotations -vs- deprecated hooks

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

Nick> In Emacs, variables get marked as obsolete but they are rarely
Nick> removed from the code.

I think that makes a lot of sense for Emacs, because there are
millions of lines of elisp out there, and breaking those randomly
would be unfriendly.

I don't think this consideration applies to gdb, though.

Nick> Creating observers just for breakpoints results in a mixed
Nick> approach for annotations as others don't use observers and
Nick> removing the deprecated label might encourage their use.

Yes.  I'm aware of the calls to annotate_* all over gdb, and the
strangely named "breakpoints_changed".

I have a followup patch to get rid of the latter.  I didn't submit it
since it causes a test suite regression :).  My approach was different
from yours in that I tried to piggy-back this on existing observers --
I'm basically trying to avoid adding observers at *every* place we
currently have a deprecated hook.  Some of those places seem like odd
spots for a hook (e.g., breakpoints_changed is called from
set_raw_breakpoint, which somehow just seems wrong to me).

Anyway, I was not actually planning to look at annotate_*; I'm
ambivalent about my breakpoints_changed patch.  I mostly wanted to get
rid of all the deprecated bits.

Nick>   http://sourceware.org/ml/gdb-patches/2008-06/msg00000.html
Nick>   http://sourceware.org/ml/gdb-patches/2008-06/msg00012.html
Nick>   http://sourceware.org/ml/gdb-patches/2008-06/msg00018.html

Oh, sorry about this.  I remembered these patches vaguely but didn't
realize how much overlap there was with my cleanups.

Anything implemented twice like this must be a good idea :-)

Tom

Re: RFA: Patch: annotations -vs- deprecated hooks

[Joel Brobecker]

> This patch removes a few deprecated hooks associated with annotations.
> This requires my previous observer patch.

I haven't had a chance to review the patch, but I really support
the idea. I know that annotations will eventually disappear, but
I think this idea will make the code a little bit better (a lot
better?), and should make it easier to keep annotations (for backward
compatibility) a little while longer. If it doesn't cost us too much,
we want to give other tools a small transition period from annotations
to MI.

I might be able to review the Tom's observers patches tomorrow.
But if I don't get to it, chances are I'll be completely tied up
until about Aug 5 :-(.

-- 
Joel

Re: RFA: Patch: annotations -vs- deprecated hooks

[Joel Brobecker]

Nick: Would you like to do some testing before this patch gets checked in?
(note that this is dependent on Tom's previous "observers" patch).

Hi Tom,

> There are still a couple of hooks called by the annotations code that
> are used by gdbtk.  This setup seems weird to me.  I guess these ought
> to be replaced by observers.

I agree - I didn't check why gdbtk is dependent on annotation hooks,
but we should also ask them about it, and try to remove these hooks.

> b/gdb/ChangeLog:
> 2008-07-13  Tom Tromey  <tromey@redhat.com>
> 
> 	* annotate.h (deprecated_annotate_starting_hook): Remove.
> 	(deprecated_annotate_stopped_hook): Remove.
> 	(deprecated_annotate_exited_hook): Remove.
> 	* Makefile.in (annotate.o): Depend on observer_h.
> 	* top.c (deprecated_delete_breakpoint_hook): Remove.
> 	(deprecated_create_breakpoint_hook): Likewise.
> 	(deprecated_modify_breakpoint_hook): Likewise.
> 	* interps.c (clear_interpreter_hooks): Update for removed hooks.
> 	* breakpoint.c (mention): Don't call removed hook.
> 	(delete_breakpoint): Likewise.
> 	(disable_breakpoint): Likewise.
> 	(do_enable_breakpoint): Likewise.
> 	* annotate.c: Include observer.h.
> 	(breakpoint_changed): Change type of argument.
> 	(_initialize_annotate): Register observers.
> 	(deprecated_annotate_starting_hook): Remove.
> 	(deprecated_annotate_stopped_hook): Remove.
> 	(deprecated_annotate_exited_hook): Remove.
> 	(annotate_starting): Update for hook removal.
> 	(annotate_stopped): Likewise.
> 	(annotate_exited): Likewise.
> 	* defs.h (deprecated_delete_breakpoint_hook): Remove.
> 	(deprecated_create_breakpoint_hook): Likewise.
> 	(deprecated_modify_breakpoint_hook): Likewise.

This looks good to me. We have to wait a little bit for Vladimir's
comments before you can commit the observers patch, so that should give
Nick a little bit of time to do any testing he might need (although
ideally running the testsuite should be enough).

-- 
Joel

Re: RFA: Patch: annotations -vs- deprecated hooks

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I agree - I didn't check why gdbtk is dependent on annotation hooks,
Joel> but we should also ask them about it, and try to remove these hooks.

I don't know why this notification was hacked into annotate.c -- but
no matter.  These notifications could easily be replaced with
observers.

I did not do that because it is not clear to me that we actually want
to directly replace all the deprecated hooks with observers.  Some of
these calls appear in weird places; maybe a more generally useful
observer could be added instead.

Tom

Re: RFA: Patch: annotations -vs- deprecated hooks

[Nick Roberts]

 > Nick: Would you like to do some testing before this patch gets checked in?
 > (note that this is dependent on Tom's previous "observers" patch).

I'm quite happy to just test it after the changes have been committed.  Any
problems are likely to be minor and, if necessary, could be fixed then.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: RFA: Patch: annotations -vs- deprecated hooks

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

[...]

Joel> This looks good to me. We have to wait a little bit for Vladimir's
Joel> comments before you can commit the observers patch, so that should give
Joel> Nick a little bit of time to do any testing he might need (although
Joel> ideally running the testsuite should be enough).

The prerequisite for this patch has gone in, so I will commit this one
shortly.

thanks,
Tom