RFC: make thread_list static

RFC: make thread_list static

[Tom Tromey]

I've had this patch on a branch for a while and thought I would send it
today.

I noticed that thread_list is only used in one place outside of
thread.c.  It seems generally preferable to me to keep things like this
private.  So, this patch makes it static and updates the one user.

Bootstrapped and regtested on x86-64 Fedora 16.

Let me know what you think.

Tom

2012-11-27  Tom Tromey  <tromey@redhat.com>

	* thread.c (thread_list): Now static.
	* remote.c (struct pending_resumption_data): New.
	(pending_thread_callback): New function.
	(append_pending_thread_resumptions): Use iterate_over_threads.
	* gdbthread.h (ALL_THREADS): Remove.
	(thread_list): Don't declare.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 0250555..7cd66b6 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -301,11 +301,6 @@ void thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid);
 typedef int (*thread_callback_func) (struct thread_info *, void *);
 extern struct thread_info *iterate_over_threads (thread_callback_func, void *);
 
-/* Traverse all threads.  */
-
-#define ALL_THREADS(T)				\
-  for (T = thread_list; T; T = T->next)
-
 extern int thread_count (void);
 
 /* Switch from one thread to another.  */
@@ -396,6 +391,4 @@ extern struct thread_info* inferior_thread (void);
 
 extern void update_thread_list (void);
 
-extern struct thread_info *thread_list;
-
 #endif /* GDBTHREAD_H */
diff --git a/gdb/remote.c b/gdb/remote.c
index 929d4f5..a67634d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4665,26 +4665,53 @@ append_resumption (char *p, char *endp,
   return p;
 }
 
+/* A structure to pass data from append_pending_thread_resumptions to
+   its worker.  */
+
+struct pending_resumption_data
+{
+  ptid_t ptid;
+  char *result;
+  char *endp;
+};
+
+/* A callback for iterate_over_threads that does the work for
+   append_pending_thread_resumptions.  */
+
+static int
+pending_thread_callback (struct thread_info *thread, void *arg)
+{
+  struct pending_resumption_data *data = arg;
+
+  if (ptid_match (thread->ptid, data->ptid)
+      && !ptid_equal (inferior_ptid, thread->ptid)
+      && thread->suspend.stop_signal != GDB_SIGNAL_0
+      && signal_pass_state (thread->suspend.stop_signal))
+    {
+      data->result = append_resumption (data->result, data->endp, thread->ptid,
+					0, thread->suspend.stop_signal);
+      thread->suspend.stop_signal = GDB_SIGNAL_0;
+    }
+
+  /* Keep going.  */
+  return 0;
+}
+
 /* Append a vCont continue-with-signal action for threads that have a
    non-zero stop signal.  */
 
 static char *
 append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
 {
-  struct thread_info *thread;
+  struct pending_resumption_data data;
 
-  ALL_THREADS (thread)
-    if (ptid_match (thread->ptid, ptid)
-	&& !ptid_equal (inferior_ptid, thread->ptid)
-	&& thread->suspend.stop_signal != GDB_SIGNAL_0
-	&& signal_pass_state (thread->suspend.stop_signal))
-      {
-	p = append_resumption (p, endp, thread->ptid,
-			       0, thread->suspend.stop_signal);
-	thread->suspend.stop_signal = GDB_SIGNAL_0;
-      }
+  data.ptid = ptid;
+  data.result = p;
+  data.endp = endp;
 
-  return p;
+  iterate_over_threads (pending_thread_callback, &data);
+
+  return data.result;
 }
 
 /* Resume the remote inferior by using a "vCont" packet.  The thread
diff --git a/gdb/thread.c b/gdb/thread.c
index 7e8eec5..ba4cbc8 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -54,7 +54,7 @@ void _initialize_thread (void);
 
 /* Prototypes for local functions.  */
 
-struct thread_info *thread_list = NULL;
+static struct thread_info *thread_list = NULL;
 static int highest_thread_num;
 
 static void thread_command (char *tidstr, int from_tty);

Re: RFC: make thread_list static

[Joel Brobecker]

On Tue, Nov 27, 2012 at 11:30:19AM -0700, Tom Tromey wrote:
> I've had this patch on a branch for a while and thought I would send it
> today.
> 
> I noticed that thread_list is only used in one place outside of
> thread.c.  It seems generally preferable to me to keep things like this
> private.  So, this patch makes it static and updates the one user.
> 
> Bootstrapped and regtested on x86-64 Fedora 16.
> 
> Let me know what you think.

I like it! I think it's a good idea to shield these globals, and
the use of iterate_over_threads isn't making the code that worse.
We're using these kinds of iterators all over the code, so it should
have a familiar feel.

> 2012-11-27  Tom Tromey  <tromey@redhat.com>
> 
> 	* thread.c (thread_list): Now static.
> 	* remote.c (struct pending_resumption_data): New.
> 	(pending_thread_callback): New function.
> 	(append_pending_thread_resumptions): Use iterate_over_threads.
> 	* gdbthread.h (ALL_THREADS): Remove.
> 	(thread_list): Don't declare.

-- 
Joel

Re: RFC: make thread_list static

[Yao Qi]

On 11/28/2012 02:30 AM, Tom Tromey wrote:
> -/* Traverse all threads.  */
> -
> -#define ALL_THREADS(T)				\
> -  for (T = thread_list; T; T = T->next)
> -

We had ITSET patches which use this macro heavily.  However they don't 
go in, I am not sure it is a good evidence of the usefulness of this macro.

Of course, the same can be achieved by iterator functions.

-- 
Yao (齐尧)

Re: RFC: make thread_list static

[Pedro Alves]

On 11/27/2012 06:30 PM, Tom Tromey wrote:
> I've had this patch on a branch for a while and thought I would send it
> today.
> 
> I noticed that thread_list is only used in one place outside of
> thread.c.  It seems generally preferable to me to keep things like this
> private.  So, this patch makes it static and updates the one user.

Well, if you ask me, I can't agree since I had added it recently (git blame
points at last July).  :-)

This was after getting sick of writing callback-style iterators in the itsets
run-control work, and introducing the macro there first.  This isn't different
from ALL_OBJFILES and several other similar macros in the tree (which I realize
from this that you'd rather remove).  We could switch to callback-style before pushing
those patches in, but, IMO, the convenience and readability of the resulting code
trumps the implementation detail exposure detail.

-- 
Pedro Alves

Re: RFC: make thread_list static

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This was after getting sick of writing callback-style iterators
Pedro> in the itsets run-control work, and introducing the macro there
Pedro> first.  This isn't different from ALL_OBJFILES and several other
Pedro> similar macros in the tree (which I realize from this that you'd
Pedro> rather remove).  We could switch to callback-style before pushing
Pedro> those patches in, but, IMO, the convenience and readability of
Pedro> the resulting code trumps the implementation detail exposure
Pedro> detail.

Ok.  I won't put it in.

FWIW -- I am not really a fan of the callback style.  It is too verbose,
usually requiring a new struct and marshalling and unmarshalling, which
means more places to forget something.

But, I'm also not a fan of the ALL_* macro approach.  I think it
obscures the code.

On the whole I'd prefer iterator objects along the lines of
dictionary.h.  These are a mild pain since in C the iterator object has
to be fully visible, but some discipline and/or field-uglification
approaches can deal with that adequately.

Tom