Re: [patch:MI] Observer for thread-changed

[Nick Roberts <nickrob at snap dot net dot nz>]

 > > Index: infrun.c
 > > ===================================================================
 > > RCS file: /cvs/src/src/gdb/infrun.c,v
 > > retrieving revision 1.278
 > > diff -p -u -p -r1.278 infrun.c
 > > --- infrun.c????6 Jun 2008 00:33:52 -0000???????1.278
 > > +++ infrun.c????9 Jun 2008 12:13:25 -0000
 > > @@ -3605,6 +3605,7 @@ normal_stop (void)
 > > ? ? ? ?target_terminal_ours_for_output ();
 > > ? ? ? ?printf_filtered (_("[Switching to %s]\n"),
 > > ???????????????? ? ? ? target_pid_to_str (inferior_ptid));
 > > + ? ? ?observer_notify_thread_changed ();
 > > ? ? ? ?annotate_thread_changed ();
 > > ? ? ? ?previous_inferior_ptid = inferior_ptid;
 > > ? ? ?}
 > 
 > Pedro's has asked if we'd want to call the observer only when we've
 > selected a frame in the new thread. Have you decided if that's a good
 > idea or not?

My observer doesn't use the selected frame, so I don't have an opinion.

 > > ...
 > > +static void
 > > +mi_thread_changed ()
 > > +{
 > > + ?struct mi_interp *mi = top_level_interpreter_data ();
 > > + ?struct interp *interp_to_use;
 > > + ?struct ui_out *old_uiout, *temp_uiout;
 > > + ?int version;
 > > +
 > > + ?fprintf_unfiltered (mi->event_channel, "thread-changed");
 > > + ?interp_to_use = top_level_interpreter ();
 > > + ?old_uiout = uiout;
 > > + ?temp_uiout = interp_ui_out (interp_to_use);
 > > + ?version = mi_version (temp_uiout);
 > > + ?temp_uiout = mi_out_new (version);
 > > + ?uiout = temp_uiout;
 > > + ?ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
 > > + ?mi_out_put (uiout, mi->event_channel);
 > > + ?uiout = old_uiout;
 > > + ?gdb_flush (mi->event_channel);
 > > +}
 > 
 > Since your other patches have similar code, how about introducing a helper
 > function for creating temporary uiout, as part of this patch?

Refactoring requires pointers to functions and function arguments and makes the
code harder to read.  I am happy to do this when there is more than one such
function.

 > > @@ -241,6 +242,7 @@ mi_cmd_thread_select (char *command, cha
 > > ? ? ?error ("mi_cmd_thread_select: USAGE: threadnum.");
 > > ?
 > > ? ?rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
 > > + ?observer_notify_thread_changed ();
 > 
 > As I've explained in the other email, I think that thread-changed
 > notification should not be emitted for -thread-select, since the frontend
 > does not need notification about something it just explicitly did
 > itself. So, this call probably should disappear.

When Emacs migrates fully to MI I anticipate it would just process the
the notification and ignore the direct output from -thread-select.  That would
mean that -thread-select and the CLI "thread" command are processed the same
way.

 > OK with the above changes.
 > 
 > Are you planning to write tests, and document the new notification?

There certainly should be tests and documentation, and I will do write some,
but I don't see any for the existing notifications, namely "thread-created" and
"thread-exited".

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