[PATCH 0/6] Refactoring around inferior continuations

[PATCH 0/6] Refactoring around inferior continuations

[Tankut Baris Aktemur]

Hi,

This is a refactoring/clean-up of the inferior continuations.

Thanks
Baris

Tankut Baris Aktemur (6):
  gdb/infcmd: remove the unused parameter 'args' in 'attach_post_wait'
  gdb/infcmd: update the comment for 'attach_post_wait'
  gdb/continuations: remove the 'err' from
    'do_all_inferior_continuations'
  gdb/continuations: do minor cleanup
  gdb/continuations: use lambdas instead of function pointers
  gdb/continuations: turn continuation functions into inferior methods

 gdb/Makefile.in     |   1 -
 gdb/continuations.c | 134 --------------------------------------------
 gdb/continuations.h |  55 ------------------
 gdb/event-top.c     |   1 -
 gdb/inf-loop.c      |   3 +-
 gdb/infcmd.c        |  65 +++++----------------
 gdb/inferior.c      |  20 ++++++-
 gdb/inferior.h      |  19 +++++--
 gdb/interps.c       |   1 -
 9 files changed, 46 insertions(+), 253 deletions(-)
 delete mode 100644 gdb/continuations.c
 delete mode 100644 gdb/continuations.h

-- 
2.17.1

[PATCH 1/6] gdb/infcmd: remove the unused parameter 'args' in 'attach_post_wait'

[Tankut Baris Aktemur]

gdb/ChangeLog:
2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infcmd.c (attach_post_wait): Remove the unused parameter 'args'.
	Update the references below.
	(struct attach_command_continuation_args)
	(attach_command_continuation)
	(attach_command_continuation_free_args)
	(attach_command)
	(notice_new_inferior): Update to remove the reference to 'args'.
---
 gdb/infcmd.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 60f25d24686..f89eb7beb03 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2471,7 +2471,7 @@ enum attach_post_wait_mode
    should be running.  Else if ATTACH, */
 
 static void
-attach_post_wait (const char *args, int from_tty, enum attach_post_wait_mode mode)
+attach_post_wait (int from_tty, enum attach_post_wait_mode mode)
 {
   struct inferior *inferior;
 
@@ -2542,7 +2542,6 @@ attach_post_wait (const char *args, int from_tty, enum attach_post_wait_mode mod
 
 struct attach_command_continuation_args
 {
-  char *args;
   int from_tty;
   enum attach_post_wait_mode mode;
 };
@@ -2556,7 +2555,7 @@ attach_command_continuation (void *args, int err)
   if (err)
     return;
 
-  attach_post_wait (a->args, a->from_tty, a->mode);
+  attach_post_wait (a->from_tty, a->mode);
 }
 
 static void
@@ -2565,7 +2564,6 @@ attach_command_continuation_free_args (void *args)
   struct attach_command_continuation_args *a
     = (struct attach_command_continuation_args *) args;
 
-  xfree (a->args);
   xfree (a);
 }
 
@@ -2677,7 +2675,6 @@ attach_command (const char *args, int from_tty)
 
       /* Wait for stop.  */
       a = XNEW (struct attach_command_continuation_args);
-      a->args = xstrdup (args);
       a->from_tty = from_tty;
       a->mode = mode;
       add_inferior_continuation (attach_command_continuation, a,
@@ -2692,7 +2689,7 @@ attach_command (const char *args, int from_tty)
       return;
     }
   else
-    attach_post_wait (args, from_tty, mode);
+    attach_post_wait (from_tty, mode);
 
   disable_commit_resumed.reset_and_commit ();
 }
@@ -2737,7 +2734,6 @@ notice_new_inferior (thread_info *thr, int leave_running, int from_tty)
 
       /* Wait for stop before proceeding.  */
       a = XNEW (struct attach_command_continuation_args);
-      a->args = xstrdup ("");
       a->from_tty = from_tty;
       a->mode = mode;
       add_inferior_continuation (attach_command_continuation, a,
@@ -2746,7 +2742,7 @@ notice_new_inferior (thread_info *thr, int leave_running, int from_tty)
       return;
     }
 
-  attach_post_wait ("" /* args */, from_tty, mode);
+  attach_post_wait (from_tty, mode);
 }
 
 /*
-- 
2.17.1

[PATCH 2/6] gdb/infcmd: update the comment for 'attach_post_wait'

[Tankut Baris Aktemur]

gdb/ChangeLog:
2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infcmd.c (attach_post_wait): Update the function comment.
---
 gdb/infcmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f89eb7beb03..3439568be00 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2467,8 +2467,8 @@ enum attach_post_wait_mode
 };
 
 /* Called after we've attached to a process and we've seen it stop for
-   the first time.  If ASYNC_EXEC is true, re-resume threads that
-   should be running.  Else if ATTACH, */
+   the first time.  Resume, stop, or don't touch the threads according
+   to MODE.  */
 
 static void
 attach_post_wait (int from_tty, enum attach_post_wait_mode mode)
@@ -2486,7 +2486,7 @@ attach_post_wait (int from_tty, enum attach_post_wait_mode mode)
       /* The user requested an `attach&', so be sure to leave threads
 	 that didn't get a signal running.  */
 
-      /* Immediatelly resume all suspended threads of this inferior,
+      /* Immediately resume all suspended threads of this inferior,
 	 and this inferior only.  This should have no effect on
 	 already running threads.  If a thread has been stopped with a
 	 signal, leave it be.  */
-- 
2.17.1

[PATCH 3/6] gdb/continuations: remove the 'err' from 'do_all_inferior_continuations'

[Tankut Baris Aktemur]

The 'err' parameter of 'do_all_inferior_continuations' is effectively
unused.  There is only one place where the function is called, and
there the argument is a literal 0.  So, remove the parameter.

gdb/ChangeLog:
2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* continuations.h (do_all_inferior_continuations): Remove the 'err'
	parameter.  Update the references below.
	* continuations.c (do_my_continuations_1)
	(do_my_continuations)
	(do_all_inferior_continuations): Update.
	* inf-loop.c (inferior_event_handler): Update.
	* infcmd.c (attach_command_continuation): Update.
---
 gdb/continuations.c | 12 ++++++------
 gdb/continuations.h | 12 +++---------
 gdb/inf-loop.c      |  2 +-
 gdb/infcmd.c        |  5 +----
 4 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/gdb/continuations.c b/gdb/continuations.c
index 78eb53f860d..8fe81a75ba9 100644
--- a/gdb/continuations.c
+++ b/gdb/continuations.c
@@ -49,14 +49,14 @@ make_continuation (struct continuation **pmy_chain,
 }
 
 static void
-do_my_continuations_1 (struct continuation **pmy_chain, int err)
+do_my_continuations_1 (struct continuation **pmy_chain)
 {
   struct continuation *ptr;
 
   while ((ptr = *pmy_chain) != NULL)
     {
       *pmy_chain = ptr->next;	/* Do this first in case of recursion.  */
-      (*ptr->function) (ptr->arg, err);
+      (*ptr->function) (ptr->arg);
       if (ptr->free_arg)
 	(*ptr->free_arg) (ptr->arg);
       xfree (ptr);
@@ -64,7 +64,7 @@ do_my_continuations_1 (struct continuation **pmy_chain, int err)
 }
 
 static void
-do_my_continuations (struct continuation **list, int err)
+do_my_continuations (struct continuation **list)
 {
   struct continuation *continuations;
 
@@ -80,7 +80,7 @@ do_my_continuations (struct continuation **list, int err)
   *list = NULL;
 
   /* Work now on the list we have set aside.  */
-  do_my_continuations_1 (&continuations, err);
+  do_my_continuations_1 (&continuations);
 }
 
 static void
@@ -119,10 +119,10 @@ add_inferior_continuation (continuation_ftype *hook, void *args,
 /* Do all continuations of the current inferior.  */
 
 void
-do_all_inferior_continuations (int err)
+do_all_inferior_continuations ()
 {
   struct inferior *inf = current_inferior ();
-  do_my_continuations (&inf->continuations, err);
+  do_my_continuations (&inf->continuations);
 }
 
 /* Get rid of all the inferior-wide continuations of INF.  */
diff --git a/gdb/continuations.h b/gdb/continuations.h
index 73f01ece132..7ebe82af1c5 100644
--- a/gdb/continuations.h
+++ b/gdb/continuations.h
@@ -30,14 +30,8 @@ struct inferior;
 
 /* Prototype of the continuation callback functions.  ARG is the
    continuation argument registered in the corresponding
-   add_*_continuation call.  ERR is true when the command should be
-   cancelled instead of finished normally.  In that case, the
-   continuation should clean up whatever state had been set up for the
-   command in question (e.g., remove momentary breakpoints).  This
-   happens e.g., when an error was thrown while handling a target
-   event, or when the inferior thread the command was being executed
-   on exits.  */
-typedef void (continuation_ftype) (void *arg, int err);
+   add_*_continuation call.  */
+typedef void (continuation_ftype) (void *arg);
 
 /* Prototype of the function responsible for releasing the argument
    passed to the continuation callback functions, either when the
@@ -49,7 +43,7 @@ typedef void (continuation_free_arg_ftype) (void *);
 extern void add_inferior_continuation (continuation_ftype *,
 				       void *,
 				       continuation_free_arg_ftype *);
-extern void do_all_inferior_continuations (int err);
+extern void do_all_inferior_continuations ();
 extern void discard_all_inferior_continuations (struct inferior *inf);
 
 #endif
diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index 9f038b3d85a..b8f66c308d2 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -55,7 +55,7 @@ inferior_event_handler (enum inferior_event_type event_type)
       /* Do all continuations associated with the whole inferior (not
 	 a particular thread).  */
       if (inferior_ptid != null_ptid)
-	do_all_inferior_continuations (0);
+	do_all_inferior_continuations ();
 
       /* When running a command list (from a user command, say), these
 	 are only run when the command list is all done.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3439568be00..5c3ee02cb9d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2547,14 +2547,11 @@ struct attach_command_continuation_args
 };
 
 static void
-attach_command_continuation (void *args, int err)
+attach_command_continuation (void *args)
 {
   struct attach_command_continuation_args *a
     = (struct attach_command_continuation_args *) args;
 
-  if (err)
-    return;
-
   attach_post_wait (a->from_tty, a->mode);
 }
 
-- 
2.17.1

[PATCH 4/6] gdb/continuations: do minor cleanup

[Tankut Baris Aktemur]

Inferior continuations are no longer used by the until and finish
command.  It is used only by the attach command and the remote target
upon detecting new inferiors.  Update the comment accordingly.

Also update another comment about non-existent thread continuations and
remove an unused #include.

gdb/ChangeLog:
2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* continuations.h: Update the general comment.
	* inferior.h (class inferior) <continuations>: Update the comment.
	* interps.c: Do not include "continuations.h".
---
 gdb/continuations.h | 4 ++--
 gdb/inferior.h      | 3 +--
 gdb/interps.c       | 1 -
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/gdb/continuations.h b/gdb/continuations.h
index 7ebe82af1c5..39130c65f0f 100644
--- a/gdb/continuations.h
+++ b/gdb/continuations.h
@@ -25,8 +25,8 @@ struct inferior;
 /* To continue the execution commands when running gdb asynchronously.
    A continuation structure contains a pointer to a function to be called
    to finish the command, once the target has stopped.  Such mechanism is
-   used by the finish and until commands, and in the remote protocol
-   when opening an extended-remote connection.  */
+   used by the attach command and the remote target when a new inferior
+   is detected.  */
 
 /* Prototype of the continuation callback functions.  ARG is the
    continuation argument registered in the corresponding
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 66fc180ce53..9ca510e4e6e 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -509,8 +509,7 @@ class inferior : public refcounted_object
   bool detaching = false;
 
   /* What is left to do for an execution command after any thread of
-     this inferior stops.  For continuations associated with a
-     specific thread, see `struct thread_info'.  */
+     this inferior stops.  */
   continuation *continuations = NULL;
 
   /* True if setup_inferior wasn't called for this inferior yet.
diff --git a/gdb/interps.c b/gdb/interps.c
index d15a36a4266..ec19822b571 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -37,7 +37,6 @@
 #include "interps.h"
 #include "completer.h"
 #include "top.h"		/* For command_loop.  */
-#include "continuations.h"
 #include "main.h"
 
 /* Each UI has its own independent set of interpreters.  */
-- 
2.17.1

[PATCH 5/6] gdb/continuations: use lambdas instead of function pointers

[Tankut Baris Aktemur]

Use lambdas and std::list to track inferior continuations.  This is a
refactoring.

gdb/ChangeLog:
2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* inferior.h (class inferior) <continuations>: Change the type
	to be an std::list of std::function's.
	Update the references and uses below.
	* continuations.c (struct continuation): Delete.
	(make_continuation): Delete.
	(do_my_continuations_1): Delete.
	(do_my_continuations): Delete.
	(discard_my_continuations_1): Delete.
	(discard_my_continuations): Delete.
	(add_inferior_continuation): Update.
	(do_all_inferior_continuations): Update.
	(discard_all_inferior_continuations): Update.
	* continuations.h (add_inferior_continuation): Update to take
	an std::function as the parameter.
	* infcmd.c (struct attach_command_continuation_args): Delete.
	(attach_command_continuation): Delete.
	(attach_command_continuation_free_args): Delete.
	(attach_command): Update.
	(notice_new_inferior): Update.
---
 gdb/continuations.c | 96 +++++----------------------------------------
 gdb/continuations.h | 24 ++++--------
 gdb/infcmd.c        | 45 ++++-----------------
 gdb/inferior.h      |  4 +-
 4 files changed, 26 insertions(+), 143 deletions(-)

diff --git a/gdb/continuations.c b/gdb/continuations.c
index 8fe81a75ba9..a552203e106 100644
--- a/gdb/continuations.c
+++ b/gdb/continuations.c
@@ -22,98 +22,15 @@
 #include "inferior.h"
 #include "continuations.h"
 
-struct continuation
-{
-  struct continuation *next;
-  continuation_ftype *function;
-  continuation_free_arg_ftype *free_arg;
-  void *arg;
-};
-
-/* Add a new continuation to the continuation chain.  Args are
-   FUNCTION to run the continuation up with, and ARG to pass to
-   it.  */
-
-static void
-make_continuation (struct continuation **pmy_chain,
-		   continuation_ftype *function,
-		   void *arg,  void (*free_arg) (void *))
-{
-  struct continuation *newobj = XNEW (struct continuation);
-
-  newobj->next = *pmy_chain;
-  newobj->function = function;
-  newobj->free_arg = free_arg;
-  newobj->arg = arg;
-  *pmy_chain = newobj;
-}
-
-static void
-do_my_continuations_1 (struct continuation **pmy_chain)
-{
-  struct continuation *ptr;
-
-  while ((ptr = *pmy_chain) != NULL)
-    {
-      *pmy_chain = ptr->next;	/* Do this first in case of recursion.  */
-      (*ptr->function) (ptr->arg);
-      if (ptr->free_arg)
-	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
-    }
-}
-
-static void
-do_my_continuations (struct continuation **list)
-{
-  struct continuation *continuations;
-
-  if (*list == NULL)
-    return;
-
-  /* Copy the list header into another pointer, and set the global
-     list header to null, so that the global list can change as a side
-     effect of invoking the continuations and the processing of the
-     preexisting continuations will not be affected.  */
-
-  continuations = *list;
-  *list = NULL;
-
-  /* Work now on the list we have set aside.  */
-  do_my_continuations_1 (&continuations);
-}
-
-static void
-discard_my_continuations_1 (struct continuation **pmy_chain)
-{
-  struct continuation *ptr;
-
-  while ((ptr = *pmy_chain) != NULL)
-    {
-      *pmy_chain = ptr->next;
-      if (ptr->free_arg)
-	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
-    }
-}
-
-static void
-discard_my_continuations (struct continuation **list)
-{
-  discard_my_continuations_1 (list);
-  *list = NULL;
-}
-
 /* Add a continuation to the continuation list of INFERIOR.  The new
    continuation will be added at the front.  */
 
 void
-add_inferior_continuation (continuation_ftype *hook, void *args,
-			   continuation_free_arg_ftype *free_arg)
+add_inferior_continuation (std::function<void ()> &&cont)
 {
   struct inferior *inf = current_inferior ();
 
-  make_continuation (&inf->continuations, hook, args, free_arg);
+  inf->continuations.emplace_front (std::move (cont));
 }
 
 /* Do all continuations of the current inferior.  */
@@ -122,7 +39,12 @@ void
 do_all_inferior_continuations ()
 {
   struct inferior *inf = current_inferior ();
-  do_my_continuations (&inf->continuations);
+  while (!inf->continuations.empty ())
+    {
+      auto &cont = inf->continuations.front ();
+      inf->continuations.pop_front ();
+      cont ();
+    }
 }
 
 /* Get rid of all the inferior-wide continuations of INF.  */
@@ -130,5 +52,5 @@ do_all_inferior_continuations ()
 void
 discard_all_inferior_continuations (struct inferior *inf)
 {
-  discard_my_continuations (&inf->continuations);
+  inf->continuations.clear ();
 }
diff --git a/gdb/continuations.h b/gdb/continuations.h
index 39130c65f0f..4dc15494ae1 100644
--- a/gdb/continuations.h
+++ b/gdb/continuations.h
@@ -20,29 +20,19 @@
 #ifndef CONTINUATIONS_H
 #define CONTINUATIONS_H
 
+#include <functional>
+
 struct inferior;
 
 /* To continue the execution commands when running gdb asynchronously.
-   A continuation structure contains a pointer to a function to be called
-   to finish the command, once the target has stopped.  Such mechanism is
-   used by the attach command and the remote target when a new inferior
-   is detected.  */
-
-/* Prototype of the continuation callback functions.  ARG is the
-   continuation argument registered in the corresponding
-   add_*_continuation call.  */
-typedef void (continuation_ftype) (void *arg);
-
-/* Prototype of the function responsible for releasing the argument
-   passed to the continuation callback functions, either when the
-   continuation is called, or discarded.  */
-typedef void (continuation_free_arg_ftype) (void *);
+   A continuation is a closure (i.e.  a lambda) to be called to finish
+   the command, once the target has stopped.  Such mechanism is used
+   by the attach command and the remote target when a new inferior is
+   detected.  */
 
 /* Inferior specific (any thread) continuations.  */
 
-extern void add_inferior_continuation (continuation_ftype *,
-				       void *,
-				       continuation_free_arg_ftype *);
+extern void add_inferior_continuation (std::function<void ()> &&cont);
 extern void do_all_inferior_continuations ();
 extern void discard_all_inferior_continuations (struct inferior *inf);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5c3ee02cb9d..e06db492b07 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2540,30 +2540,6 @@ attach_post_wait (int from_tty, enum attach_post_wait_mode mode)
     }
 }
 
-struct attach_command_continuation_args
-{
-  int from_tty;
-  enum attach_post_wait_mode mode;
-};
-
-static void
-attach_command_continuation (void *args)
-{
-  struct attach_command_continuation_args *a
-    = (struct attach_command_continuation_args *) args;
-
-  attach_post_wait (a->from_tty, a->mode);
-}
-
-static void
-attach_command_continuation_free_args (void *args)
-{
-  struct attach_command_continuation_args *a
-    = (struct attach_command_continuation_args *) args;
-
-  xfree (a);
-}
-
 /* "attach" command entry point.  Takes a program started up outside
    of gdb and ``attaches'' to it.  This stops it cold in its tracks
    and allows us to start debugging it.  */
@@ -2661,8 +2637,6 @@ attach_command (const char *args, int from_tty)
      E.g. Mach 3 or GNU hurd.  */
   if (!target_attach_no_wait ())
     {
-      struct attach_command_continuation_args *a;
-
       /* Careful here.  See comments in inferior.h.  Basically some
 	 OSes don't ignore SIGSTOPs on continue requests anymore.  We
 	 need a way for handle_inferior_event to reset the stop_signal
@@ -2671,11 +2645,10 @@ attach_command (const char *args, int from_tty)
       inferior->control.stop_soon = STOP_QUIETLY_NO_SIGSTOP;
 
       /* Wait for stop.  */
-      a = XNEW (struct attach_command_continuation_args);
-      a->from_tty = from_tty;
-      a->mode = mode;
-      add_inferior_continuation (attach_command_continuation, a,
-				 attach_command_continuation_free_args);
+      add_inferior_continuation ([=] ()
+	{
+	  attach_post_wait (from_tty, mode);
+	});
 
       /* Let infrun consider waiting for events out of this
 	 target.  */
@@ -2719,7 +2692,6 @@ notice_new_inferior (thread_info *thr, int leave_running, int from_tty)
 
   if (thr->executing)
     {
-      struct attach_command_continuation_args *a;
       struct inferior *inferior = current_inferior ();
 
       /* We're going to install breakpoints, and poke at memory,
@@ -2730,11 +2702,10 @@ notice_new_inferior (thread_info *thr, int leave_running, int from_tty)
       inferior->control.stop_soon = STOP_QUIETLY_REMOTE;
 
       /* Wait for stop before proceeding.  */
-      a = XNEW (struct attach_command_continuation_args);
-      a->from_tty = from_tty;
-      a->mode = mode;
-      add_inferior_continuation (attach_command_continuation, a,
-				 attach_command_continuation_free_args);
+      add_inferior_continuation ([=] ()
+	{
+	  attach_post_wait (from_tty, mode);
+	});
 
       return;
     }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 9ca510e4e6e..fc072b3576f 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -22,6 +22,7 @@
 #define INFERIOR_H 1
 
 #include <exception>
+#include <list>
 
 struct target_waitstatus;
 struct frame_info;
@@ -32,7 +33,6 @@ struct regcache;
 struct ui_out;
 struct terminal_info;
 struct target_desc_info;
-struct continuation;
 struct inferior;
 struct thread_info;
 
@@ -510,7 +510,7 @@ class inferior : public refcounted_object
 
   /* What is left to do for an execution command after any thread of
      this inferior stops.  */
-  continuation *continuations = NULL;
+  std::list<std::function<void ()> > continuations;
 
   /* True if setup_inferior wasn't called for this inferior yet.
      Until that is done, we must not access inferior memory or
-- 
2.17.1

[PATCH 6/6] gdb/continuations: turn continuation functions into inferior methods

[Tankut Baris Aktemur]

Turn continuations-related functions into methods of the inferior
class.  This is a refactoring.

gdb/ChangeLog:
2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* Makefile.in (COMMON_SFILES): Remove continuations.c.
	* inferior.c (inferior::add_continuation): New method, adapted
	from 'add_inferior_continuation'.
	(inferior::do_all_continuations): New method, adapted from
	'do_all_inferior_continuations'.
	(inferior::~inferior): Clear the list of continuations directly.
	* inferior.h (class inferior) <continuations>: Rename into...
	<m_continuations>: ...this and make private.
	* continuations.c: Remove.
	* continuations.h: Remove.
	* event-top.c: Don't include "continuations.h".

	Update the users below.
	* inf-loop.c (inferior_event_handler)
	* infcmd.c (attach_command)
	(notice_new_inferior): Update.
---
 gdb/Makefile.in     |  1 -
 gdb/continuations.c | 56 ---------------------------------------------
 gdb/continuations.h | 39 -------------------------------
 gdb/event-top.c     |  1 -
 gdb/inf-loop.c      |  3 +--
 gdb/infcmd.c        |  5 ++--
 gdb/inferior.c      | 20 ++++++++++++++--
 gdb/inferior.h      | 16 +++++++++----
 8 files changed, 33 insertions(+), 108 deletions(-)
 delete mode 100644 gdb/continuations.c
 delete mode 100644 gdb/continuations.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 490419030a3..ba0cabb0216 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1004,7 +1004,6 @@ COMMON_SFILES = \
 	coffread.c \
 	complaints.c \
 	completer.c \
-	continuations.c \
 	copying.c \
 	corefile.c \
 	corelow.c \
diff --git a/gdb/continuations.c b/gdb/continuations.c
deleted file mode 100644
index a552203e106..00000000000
--- a/gdb/continuations.c
+++ /dev/null
@@ -1,56 +0,0 @@
-/* Continuations for GDB, the GNU debugger.
-
-   Copyright (C) 1986-2021 Free Software Foundation, Inc.
-
-   This file is part of GDB.
-
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
-#include "defs.h"
-#include "gdbthread.h"
-#include "inferior.h"
-#include "continuations.h"
-
-/* Add a continuation to the continuation list of INFERIOR.  The new
-   continuation will be added at the front.  */
-
-void
-add_inferior_continuation (std::function<void ()> &&cont)
-{
-  struct inferior *inf = current_inferior ();
-
-  inf->continuations.emplace_front (std::move (cont));
-}
-
-/* Do all continuations of the current inferior.  */
-
-void
-do_all_inferior_continuations ()
-{
-  struct inferior *inf = current_inferior ();
-  while (!inf->continuations.empty ())
-    {
-      auto &cont = inf->continuations.front ();
-      inf->continuations.pop_front ();
-      cont ();
-    }
-}
-
-/* Get rid of all the inferior-wide continuations of INF.  */
-
-void
-discard_all_inferior_continuations (struct inferior *inf)
-{
-  inf->continuations.clear ();
-}
diff --git a/gdb/continuations.h b/gdb/continuations.h
deleted file mode 100644
index 4dc15494ae1..00000000000
--- a/gdb/continuations.h
+++ /dev/null
@@ -1,39 +0,0 @@
-/* Continuations for GDB, the GNU debugger.
-
-   Copyright (C) 1999-2021 Free Software Foundation, Inc.
-
-   This file is part of GDB.
-
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
-#ifndef CONTINUATIONS_H
-#define CONTINUATIONS_H
-
-#include <functional>
-
-struct inferior;
-
-/* To continue the execution commands when running gdb asynchronously.
-   A continuation is a closure (i.e.  a lambda) to be called to finish
-   the command, once the target has stopped.  Such mechanism is used
-   by the attach command and the remote target when a new inferior is
-   detected.  */
-
-/* Inferior specific (any thread) continuations.  */
-
-extern void add_inferior_continuation (std::function<void ()> &&cont);
-extern void do_all_inferior_continuations ();
-extern void discard_all_inferior_continuations (struct inferior *inf);
-
-#endif
diff --git a/gdb/event-top.c b/gdb/event-top.c
index fb0df943f65..002a7dc95e0 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -33,7 +33,6 @@
 #include "main.h"
 #include "gdbthread.h"
 #include "observable.h"
-#include "continuations.h"
 #include "gdbcmd.h"		/* for dont_repeat() */
 #include "annotate.h"
 #include "maint.h"
diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index b8f66c308d2..dc3ffbb27f3 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -26,7 +26,6 @@
 #include "remote.h"
 #include "language.h"
 #include "gdbthread.h"
-#include "continuations.h"
 #include "interps.h"
 #include "top.h"
 #include "observable.h"
@@ -55,7 +54,7 @@ inferior_event_handler (enum inferior_event_type event_type)
       /* Do all continuations associated with the whole inferior (not
 	 a particular thread).  */
       if (inferior_ptid != null_ptid)
-	do_all_inferior_continuations ();
+	current_inferior ()->do_all_continuations ();
 
       /* When running a command list (from a user command, say), these
 	 are only run when the command list is all done.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index e06db492b07..5aa6b00f20f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -47,7 +47,6 @@
 #include "inline-frame.h"
 #include "tracepoint.h"
 #include "inf-loop.h"
-#include "continuations.h"
 #include "linespec.h"
 #include "thread-fsm.h"
 #include "top.h"
@@ -2645,7 +2644,7 @@ attach_command (const char *args, int from_tty)
       inferior->control.stop_soon = STOP_QUIETLY_NO_SIGSTOP;
 
       /* Wait for stop.  */
-      add_inferior_continuation ([=] ()
+      inferior->add_continuation ([=] ()
 	{
 	  attach_post_wait (from_tty, mode);
 	});
@@ -2702,7 +2701,7 @@ notice_new_inferior (thread_info *thr, int leave_running, int from_tty)
       inferior->control.stop_soon = STOP_QUIETLY_REMOTE;
 
       /* Wait for stop before proceeding.  */
-      add_inferior_continuation ([=] ()
+      inferior->add_continuation ([=] ()
 	{
 	  attach_post_wait (from_tty, mode);
 	});
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 9188f72e35d..da8209548c8 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -31,7 +31,6 @@
 #include "symfile.h"
 #include "gdbsupport/environ.h"
 #include "cli/cli-utils.h"
-#include "continuations.h"
 #include "arch-utils.h"
 #include "target-descriptions.h"
 #include "readline/tilde.h"
@@ -74,7 +73,7 @@ inferior::~inferior ()
 {
   inferior *inf = this;
 
-  discard_all_inferior_continuations (inf);
+  m_continuations.clear ();
   inferior_free_data (inf);
   xfree (inf->args);
   target_desc_info_free (inf->tdesc_info);
@@ -106,6 +105,23 @@ inferior::tty ()
   return m_terminal.get ();
 }
 
+void
+inferior::add_continuation (std::function<void ()> &&cont)
+{
+  m_continuations.emplace_front (std::move (cont));
+}
+
+void
+inferior::do_all_continuations ()
+{
+  while (!m_continuations.empty ())
+    {
+      auto &cont = m_continuations.front ();
+      m_continuations.pop_front ();
+      cont ();
+    }
+}
+
 struct inferior *
 add_inferior_silent (int pid)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index fc072b3576f..258faa5db98 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -422,6 +422,15 @@ class inferior : public refcounted_object
   inline safe_inf_threads_range threads_safe ()
   { return safe_inf_threads_range (this->thread_list); }
 
+  /* Continuations-related methods.  A continuation is a closure (i.e.
+     a lambda) to be called to finish the execution of a command when
+     running GDB asynchronously.  A continuation is executed after any
+     thread of this inferior stops.  Continuations are used by the
+     attach command and the remote target when a new inferior is
+     detected.  */
+  void add_continuation (std::function<void ()> &&cont);
+  void do_all_continuations ();
+
   /* Set/get file name for default use for standard in/out in the
      inferior.  On Unix systems, we try to make TERMINAL_NAME the
      inferior's controlling terminal.  If TERMINAL_NAME is nullptr or
@@ -508,10 +517,6 @@ class inferior : public refcounted_object
   /* True if we're in the process of detaching from this inferior.  */
   bool detaching = false;
 
-  /* What is left to do for an execution command after any thread of
-     this inferior stops.  */
-  std::list<std::function<void ()> > continuations;
-
   /* True if setup_inferior wasn't called for this inferior yet.
      Until that is done, we must not access inferior memory or
      registers, as we haven't determined the target
@@ -559,6 +564,9 @@ class inferior : public refcounted_object
 
   /* The name of terminal device to use for I/O.  */
   gdb::unique_xmalloc_ptr<char> m_terminal;
+
+  /* The list of continuations.  */
+  std::list<std::function<void ()> > m_continuations;
 };
 
 /* Keep a registry of per-inferior data-pointers required by other GDB
-- 
2.17.1

Re: [PATCH 1/6] gdb/infcmd: remove the unused parameter 'args' in 'attach_post_wait'

[Tom Tromey]

>>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

>> gdb/ChangeLog:
>> 2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

>> 	* infcmd.c (attach_post_wait): Remove the unused parameter 'args'.
>> 	Update the references below.
>> 	(struct attach_command_continuation_args)
>> 	(attach_command_continuation)
>> 	(attach_command_continuation_free_args)
>> 	(attach_command)
>> 	(notice_new_inferior): Update to remove the reference to 'args'.

Normally it's good to have some text in the body that explains the
commit.

The patch is ok though.

thank you,
Tom

Re: [PATCH 2/6] gdb/infcmd: update the comment for 'attach_post_wait'

[Tom Tromey]

>>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

Tankut> gdb/ChangeLog:
Tankut> 2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

Tankut> 	* infcmd.c (attach_post_wait): Update the function comment.

Ok.

Tom

Re: [PATCH 3/6] gdb/continuations: remove the 'err' from 'do_all_inferior_continuations'

[Tom Tromey]

>>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

>> The 'err' parameter of 'do_all_inferior_continuations' is effectively
>> unused.  There is only one place where the function is called, and
>> there the argument is a literal 0.  So, remove the parameter.

>> gdb/ChangeLog:
>> 2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

>> 	* continuations.h (do_all_inferior_continuations): Remove the 'err'
>> 	parameter.  Update the references below.
>> 	* continuations.c (do_my_continuations_1)
>> 	(do_my_continuations)
>> 	(do_all_inferior_continuations): Update.
>> 	* inf-loop.c (inferior_event_handler): Update.
>> 	* infcmd.c (attach_command_continuation): Update.

Looks good.  Thank you.

Tom

Re: [PATCH 4/6] gdb/continuations: do minor cleanup

[Tom Tromey]

>>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

>> Inferior continuations are no longer used by the until and finish
>> command.  It is used only by the attach command and the remote target
>> upon detecting new inferiors.  Update the comment accordingly.

>> Also update another comment about non-existent thread continuations and
>> remove an unused #include.

>> gdb/ChangeLog:
>> 2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

>> 	* continuations.h: Update the general comment.
>> 	* inferior.h (class inferior) <continuations>: Update the comment.
>> 	* interps.c: Do not include "continuations.h".

Thank you.  This is ok.

Tom

Re: [PATCH 1/6] gdb/infcmd: remove the unused parameter 'args' in 'attach_post_wait'

[Simon Marchi]

On 2021-04-21 3:24 p.m., Tom Tromey wrote:
>>>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>> gdb/ChangeLog:
>>> 2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
>>> 	* infcmd.c (attach_post_wait): Remove the unused parameter 'args'.
>>> 	Update the references below.
>>> 	(struct attach_command_continuation_args)
>>> 	(attach_command_continuation)
>>> 	(attach_command_continuation_free_args)
>>> 	(attach_command)
>>> 	(notice_new_inferior): Update to remove the reference to 'args'.
> 
> Normally it's good to have some text in the body that explains the
> commit.

I agree with you in general, but when I do patches like this one (remove
unused parameter / variable), I also find it hard to find anything else
to say than just repeating the subject.

Simon

Re: [PATCH 5/6] gdb/continuations: use lambdas instead of function pointers

[Tom Tromey]

>>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

>> Use lambdas and std::list to track inferior continuations.  This is a
>> refactoring.

Thanks for the patch.

>> @@ -122,7 +39,12 @@ void
>>  do_all_inferior_continuations ()
>>  {
>>    struct inferior *inf = current_inferior ();
>> -  do_my_continuations (&inf->continuations);
>> +  while (!inf->continuations.empty ())
>> +    {
>> +      auto &cont = inf->continuations.front ();
>> +      inf->continuations.pop_front ();
>> +      cont ();

It seems like this sequence can lead to 'cont' referencing an invalid
function, because 'cont' is just a reference, but pop_front will cause
it to be destroyed.

Maybe "auto cont = std::move (...)" would work ok.
Or, delaying the pop until after the callback is called seems fine as
well.

>> +   A continuation is a closure (i.e.  a lambda) to be called to finish

I think it would be fine to say "A continuation is a std::function"
here, because that encompasses lambdas but also other valid forms, like
ordinary functions.

>> +  std::list<std::function<void ()> > continuations;
 
gdb style usually uses the ">>" token instead of adding the space here.

thanks,
Tom

Re: [PATCH 6/6] gdb/continuations: turn continuation functions into inferior methods

[Tom Tromey]

>>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

>> Turn continuations-related functions into methods of the inferior
>> class.  This is a refactoring.

>> gdb/ChangeLog:
>> 2021-04-21  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

>> 	* Makefile.in (COMMON_SFILES): Remove continuations.c.
>> 	* inferior.c (inferior::add_continuation): New method, adapted
>> 	from 'add_inferior_continuation'.
>> 	(inferior::do_all_continuations): New method, adapted from
>> 	'do_all_inferior_continuations'.
>> 	(inferior::~inferior): Clear the list of continuations directly.
>> 	* inferior.h (class inferior) <continuations>: Rename into...
>> 	<m_continuations>: ...this and make private.
>> 	* continuations.c: Remove.
>> 	* continuations.h: Remove.
>> 	* event-top.c: Don't include "continuations.h".

>> 	Update the users below.
>> 	* inf-loop.c (inferior_event_handler)
>> 	* infcmd.c (attach_command)
>> 	(notice_new_inferior): Update.

Looks good, provided the updates from the previous patch's review are
taken into account.  Thank you for doing this.

Tom

Re: [PATCH 1/6] gdb/infcmd: remove the unused parameter 'args' in 'attach_post_wait'

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I agree with you in general, but when I do patches like this one (remove
Simon> unused parameter / variable), I also find it hard to find anything else
Simon> to say than just repeating the subject.

Yeah.  I'm fine with that for the simple patches if everybody else is.

Tom

RE: [PATCH 1/6] gdb/infcmd: remove the unused parameter 'args' in 'attach_post_wait'

[Aktemur, Tankut Baris]

On Wednesday, April 21, 2021 9:48 PM, Tom Tromey wrote:
> >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> I agree with you in general, but when I do patches like this one (remove
> Simon> unused parameter / variable), I also find it hard to find anything else
> Simon> to say than just repeating the subject.
> 
> Yeah.  I'm fine with that for the simple patches if everybody else is.
> 
> Tom

I'll add a short text to the commit message body.  Thanks.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 5/6] gdb/continuations: use lambdas instead of function pointers

[Aktemur, Tankut Baris]

On Wednesday, April 21, 2021 9:44 PM, Tom Tromey wrote:
> >>>>> ">" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> >> Use lambdas and std::list to track inferior continuations.  This is a
> >> refactoring.
> 
> Thanks for the patch.
> 
> >> @@ -122,7 +39,12 @@ void
> >>  do_all_inferior_continuations ()
> >>  {
> >>    struct inferior *inf = current_inferior ();
> >> -  do_my_continuations (&inf->continuations);
> >> +  while (!inf->continuations.empty ())
> >> +    {
> >> +      auto &cont = inf->continuations.front ();
> >> +      inf->continuations.pop_front ();
> >> +      cont ();
> 
> It seems like this sequence can lead to 'cont' referencing an invalid
> function, because 'cont' is just a reference, but pop_front will cause
> it to be destroyed.

Ah, thanks for catching that.

> Maybe "auto cont = std::move (...)" would work ok.
> Or, delaying the pop until after the callback is called seems fine as
> well.

The continuations at the moment are rather plain, but just in case a
continuation in the future adds more continuations, delaying the pop
could cause a wrong element to be popped, I'm afraid.  How about the
following? (The same is to be used in Patch 6/6, too).

@@ -122,7 +39,12 @@ void
 do_all_inferior_continuations ()
 {
   struct inferior *inf = current_inferior ();
-  do_my_continuations (&inf->continuations);
+  while (!inf->continuations.empty ())
+    {
+      auto iter = inf->continuations.begin ();
+      (*iter) ();
+      inf->continuations.erase (iter);
+    }
 }


> >> +   A continuation is a closure (i.e.  a lambda) to be called to finish
> 
> I think it would be fine to say "A continuation is a std::function"
> here, because that encompasses lambdas but also other valid forms, like
> ordinary functions.

Out of curiosity, how do you pronounce 'std'?  I was just thinking if it
should be "a std::function" or "an std::function".  I pronounce std as
"ess-tee-dee" and "an" sounded more natural to me.

> >> +  std::list<std::function<void ()> > continuations;
> 
> gdb style usually uses the ">>" token instead of adding the space here.
> 
> thanks,
> Tom

Fixed.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 5/6] gdb/continuations: use lambdas instead of function pointers

[Tom Tromey]

>>>>> ">" == Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> writes:

>> The continuations at the moment are rather plain, but just in case a
>> continuation in the future adds more continuations, delaying the pop
>> could cause a wrong element to be popped, I'm afraid.  How about the
>> following? (The same is to be used in Patch 6/6, too).

>> @@ -122,7 +39,12 @@ void
>>  do_all_inferior_continuations ()
>>  {
>>    struct inferior *inf = current_inferior ();
>> -  do_my_continuations (&inf->continuations);
>> +  while (!inf->continuations.empty ())
>> +    {
>> +      auto iter = inf->continuations.begin ();
>> +      (*iter) ();
>> +      inf->continuations.erase (iter);

It took me a minute to see what was going on here, at first I misread it
a bit.  But yeah, I see, I think this is fine.

If elements are only ever added/popped from the front, maybe
std::forward_list is preferable.

>> Out of curiosity, how do you pronounce 'std'?  I was just thinking if it
>> should be "a std::function" or "an std::function".  I pronounce std as
>> "ess-tee-dee" and "an" sounded more natural to me.

Hah, I didn't even think about that.  In my head it sounds like "sted",
but that's just something I made up myself.  Either way is fine by me,
or even just not referring to the type, since that's clear from context.

Tom

RE: [PATCH 5/6] gdb/continuations: use lambdas instead of function pointers

[Aktemur, Tankut Baris]

On Thursday, April 22, 2021 2:50 PM, Tom Tromey wrote:
> >>>>> ">" == Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> writes:
> 
> >> The continuations at the moment are rather plain, but just in case a
> >> continuation in the future adds more continuations, delaying the pop
> >> could cause a wrong element to be popped, I'm afraid.  How about the
> >> following? (The same is to be used in Patch 6/6, too).
> 
> >> @@ -122,7 +39,12 @@ void
> >>  do_all_inferior_continuations ()
> >>  {
> >>    struct inferior *inf = current_inferior ();
> >> -  do_my_continuations (&inf->continuations);
> >> +  while (!inf->continuations.empty ())
> >> +    {
> >> +      auto iter = inf->continuations.begin ();
> >> +      (*iter) ();
> >> +      inf->continuations.erase (iter);
> 
> It took me a minute to see what was going on here, at first I misread it
> a bit.  But yeah, I see, I think this is fine.
> 
> If elements are only ever added/popped from the front, maybe
> std::forward_list is preferable.

If you don't mind it much, I'd like keep std::list.  With std::forward_list,
we would additionally have to use erase_after and before_begin, which make
the code uglier, in my opinion.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 5/6] gdb/continuations: use lambdas instead of function pointers

[Tom Tromey]

>>>>> Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> writes:

> If you don't mind it much, I'd like keep std::list.  With std::forward_list,
> we would additionally have to use erase_after and before_begin, which make
> the code uglier, in my opinion.

It's fine by me, thanks.

Tom