[RFA 0/2] two simple cleanup removals

[RFA 0/2] two simple cleanup removals

[Tom Tromey]

Here are a couple of simple cleanup removal patches.

These were regression tested by the buildbot.

Tom

[RFA 2/2] Remove cleanups from prompt_for_continue

[Tom Tromey]

This removes the cleanups from prompt_for_continue by the use of
unique_xmalloc_ptr.

gdb/ChangeLog
2018-03-22  Tom Tromey  <tom@tromey.com>

	* utils.c (prompt_for_continue): Use unique_xmalloc_ptr.
---
 gdb/ChangeLog | 4 ++++
 gdb/utils.c   | 9 ++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 3886efd840..ee31f39661 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1479,9 +1479,7 @@ set_screen_width_and_height (int width, int height)
 static void
 prompt_for_continue (void)
 {
-  char *ignore;
   char cont_prompt[120];
-  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   /* Used to add duration we waited for user to respond to
      prompt_for_continue_wait_time.  */
   using namespace std::chrono;
@@ -1504,8 +1502,7 @@ prompt_for_continue (void)
 
   /* Call gdb_readline_wrapper, not readline, in order to keep an
      event loop running.  */
-  ignore = gdb_readline_wrapper (cont_prompt);
-  make_cleanup (xfree, ignore);
+  gdb::unique_xmalloc_ptr<char> ignore (gdb_readline_wrapper (cont_prompt));
 
   /* Add time spend in this routine to prompt_for_continue_wait_time.  */
   prompt_for_continue_wait_time += steady_clock::now () - prompt_started;
@@ -1515,7 +1512,7 @@ prompt_for_continue (void)
 
   if (ignore != NULL)
     {
-      char *p = ignore;
+      char *p = ignore.get ();
 
       while (*p == ' ' || *p == '\t')
 	++p;
@@ -1529,8 +1526,6 @@ prompt_for_continue (void)
   reinitialize_more_filter ();
 
   dont_repeat ();		/* Forget prev cmd -- CR won't repeat it.  */
-
-  do_cleanups (old_chain);
 }
 
 /* Initialize timer to keep track of how long we waited for the user.  */
-- 
2.13.6

[RFA 1/2] Remove cleanups from gdb_readline_wrapper

[Tom Tromey]

This removes some cleanups from gdb_readline_wrapper by changing the
existing gdb_readline_wrapper_cleanup struct to have a constructor and
destructor, and then changing gdb_readline_wrapper to simply
instantiate it on the stack.

gdb/ChangeLog
2018-03-22  Tom Tromey  <tom@tromey.com>

	* top.c (struct gdb_readline_wrapper_cleanup): Add constructor,
	destructor.
	(gdb_readline_wrapper_cleanup): Remove function.
	(gdb_readline_wrapper): Remove cleanups.
---
 gdb/ChangeLog |  7 +++++
 gdb/top.c     | 91 ++++++++++++++++++++++++++---------------------------------
 2 files changed, 47 insertions(+), 51 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 14387b9f1a..02d23cca66 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -919,73 +919,64 @@ gdb_readline_wrapper_line (char *line)
 
 struct gdb_readline_wrapper_cleanup
   {
-    void (*handler_orig) (char *);
-    int already_prompted_orig;
+    explicit gdb_readline_wrapper_cleanup (struct ui *ui)
+      : handler_orig (ui->input_handler),
+	already_prompted_orig (ui->command_editing ? rl_already_prompted : 0),
+	target_is_async_orig (target_is_async_p ())
+    {
+      ui->input_handler = gdb_readline_wrapper_line;
+      ui->secondary_prompt_depth++;
+    }
 
-    /* Whether the target was async.  */
-    int target_is_async_orig;
-  };
+    ~gdb_readline_wrapper_cleanup ()
+    {
+      struct ui *ui = current_ui;
 
-static void
-gdb_readline_wrapper_cleanup (void *arg)
-{
-  struct ui *ui = current_ui;
-  struct gdb_readline_wrapper_cleanup *cleanup
-    = (struct gdb_readline_wrapper_cleanup *) arg;
+      if (ui->command_editing)
+	rl_already_prompted = already_prompted_orig;
 
-  if (ui->command_editing)
-    rl_already_prompted = cleanup->already_prompted_orig;
+      gdb_assert (ui->input_handler == gdb_readline_wrapper_line);
+      ui->input_handler = handler_orig;
 
-  gdb_assert (ui->input_handler == gdb_readline_wrapper_line);
-  ui->input_handler = cleanup->handler_orig;
+      /* Don't restore our input handler in readline yet.  That would make
+	 readline prep the terminal (putting it in raw mode), while the
+	 line we just read may trigger execution of a command that expects
+	 the terminal in the default cooked/canonical mode, such as e.g.,
+	 running Python's interactive online help utility.  See
+	 gdb_readline_wrapper_line for when we'll reinstall it.  */
 
-  /* Don't restore our input handler in readline yet.  That would make
-     readline prep the terminal (putting it in raw mode), while the
-     line we just read may trigger execution of a command that expects
-     the terminal in the default cooked/canonical mode, such as e.g.,
-     running Python's interactive online help utility.  See
-     gdb_readline_wrapper_line for when we'll reinstall it.  */
+      gdb_readline_wrapper_result = NULL;
+      gdb_readline_wrapper_done = 0;
+      ui->secondary_prompt_depth--;
+      gdb_assert (ui->secondary_prompt_depth >= 0);
 
-  gdb_readline_wrapper_result = NULL;
-  gdb_readline_wrapper_done = 0;
-  ui->secondary_prompt_depth--;
-  gdb_assert (ui->secondary_prompt_depth >= 0);
+      after_char_processing_hook = saved_after_char_processing_hook;
+      saved_after_char_processing_hook = NULL;
 
-  after_char_processing_hook = saved_after_char_processing_hook;
-  saved_after_char_processing_hook = NULL;
+      if (target_is_async_orig)
+	target_async (1);
+    }
 
-  if (cleanup->target_is_async_orig)
-    target_async (1);
+    DISABLE_COPY_AND_ASSIGN (gdb_readline_wrapper_cleanup);
 
-  xfree (cleanup);
-}
+    void (*handler_orig) (char *);
+    int already_prompted_orig;
+
+    /* Whether the target was async.  */
+    int target_is_async_orig;
+  };
 
 char *
 gdb_readline_wrapper (const char *prompt)
 {
   struct ui *ui = current_ui;
-  struct cleanup *back_to;
-  struct gdb_readline_wrapper_cleanup *cleanup;
-  char *retval;
-
-  cleanup = XNEW (struct gdb_readline_wrapper_cleanup);
-  cleanup->handler_orig = ui->input_handler;
-  ui->input_handler = gdb_readline_wrapper_line;
-
-  if (ui->command_editing)
-    cleanup->already_prompted_orig = rl_already_prompted;
-  else
-    cleanup->already_prompted_orig = 0;
-
-  cleanup->target_is_async_orig = target_is_async_p ();
 
-  ui->secondary_prompt_depth++;
-  back_to = make_cleanup (gdb_readline_wrapper_cleanup, cleanup);
+  gdb_readline_wrapper_cleanup cleanup (ui);
 
   /* Processing events may change the current UI.  */
   scoped_restore save_ui = make_scoped_restore (&current_ui);
 
-  if (cleanup->target_is_async_orig)
+  if (cleanup.target_is_async_orig)
     target_async (0);
 
   /* Display our prompt and prevent double prompt display.  Don't pass
@@ -1004,9 +995,7 @@ gdb_readline_wrapper (const char *prompt)
     if (gdb_readline_wrapper_done)
       break;
 
-  retval = gdb_readline_wrapper_result;
-  do_cleanups (back_to);
-  return retval;
+  return gdb_readline_wrapper_result;
 }
 
 \f
-- 
2.13.6

Re: [RFA 1/2] Remove cleanups from gdb_readline_wrapper

[Pedro Alves]

Hi Tom,

On 03/23/2018 03:38 AM, Tom Tromey wrote:

>  char *
>  gdb_readline_wrapper (const char *prompt)
>  {
>    struct ui *ui = current_ui;
> -  struct cleanup *back_to;
> -  struct gdb_readline_wrapper_cleanup *cleanup;
> -  char *retval;
> -
> -  cleanup = XNEW (struct gdb_readline_wrapper_cleanup);
> -  cleanup->handler_orig = ui->input_handler;
> -  ui->input_handler = gdb_readline_wrapper_line;
> -
> -  if (ui->command_editing)
> -    cleanup->already_prompted_orig = rl_already_prompted;
> -  else
> -    cleanup->already_prompted_orig = 0;
> -
> -  cleanup->target_is_async_orig = target_is_async_p ();
>  
> -  ui->secondary_prompt_depth++;
> -  back_to = make_cleanup (gdb_readline_wrapper_cleanup, cleanup);
> +  gdb_readline_wrapper_cleanup cleanup (ui);
>  
>    /* Processing events may change the current UI.  */
>    scoped_restore save_ui = make_scoped_restore (&current_ui);
>  
> -  if (cleanup->target_is_async_orig)
> +  if (cleanup.target_is_async_orig)
>      target_async (0);

I think we can move these to the ctor too, and then all the fields
of the structure can be made private.  Something like the patch
below.  (Tested on x86-64 GNU/Linux.)

While at it, since we're touching most of the structure, we might as
well reindent it to have open { at column 0.  (Not done below to keep
the patch readable.)

WDYT?

From a98b9bc98522351f757e3835b22bdfa9ecf2f997 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 24 Mar 2018 10:55:31 +0000
Subject: [PATCH] gdb_readline_wrapper_cleanup

---
 gdb/top.c | 50 +++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 02d23cca667..523530ffbae 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -917,15 +917,21 @@ gdb_readline_wrapper_line (char *line)
     gdb_rl_callback_handler_remove ();
 }
 
-struct gdb_readline_wrapper_cleanup
+class gdb_readline_wrapper_cleanup
   {
-    explicit gdb_readline_wrapper_cleanup (struct ui *ui)
-      : handler_orig (ui->input_handler),
-	already_prompted_orig (ui->command_editing ? rl_already_prompted : 0),
-	target_is_async_orig (target_is_async_p ())
+  public:
+    explicit gdb_readline_wrapper_cleanup ()
+      : m_handler_orig (current_ui->input_handler),
+	m_already_prompted_orig (current_ui->command_editing
+				 ? rl_already_prompted : 0),
+	m_target_is_async_orig (target_is_async_p ()),
+	m_save_ui (&current_ui)
     {
-      ui->input_handler = gdb_readline_wrapper_line;
-      ui->secondary_prompt_depth++;
+      current_ui->input_handler = gdb_readline_wrapper_line;
+      current_ui->secondary_prompt_depth++;
+
+      if (m_target_is_async_orig)
+	target_async (0);
     }
 
     ~gdb_readline_wrapper_cleanup ()
@@ -933,10 +939,10 @@ struct gdb_readline_wrapper_cleanup
       struct ui *ui = current_ui;
 
       if (ui->command_editing)
-	rl_already_prompted = already_prompted_orig;
+	rl_already_prompted = m_already_prompted_orig;
 
       gdb_assert (ui->input_handler == gdb_readline_wrapper_line);
-      ui->input_handler = handler_orig;
+      ui->input_handler = m_handler_orig;
 
       /* Don't restore our input handler in readline yet.  That would make
 	 readline prep the terminal (putting it in raw mode), while the
@@ -953,38 +959,36 @@ struct gdb_readline_wrapper_cleanup
       after_char_processing_hook = saved_after_char_processing_hook;
       saved_after_char_processing_hook = NULL;
 
-      if (target_is_async_orig)
+      if (m_target_is_async_orig)
 	target_async (1);
     }
 
     DISABLE_COPY_AND_ASSIGN (gdb_readline_wrapper_cleanup);
 
-    void (*handler_orig) (char *);
-    int already_prompted_orig;
+  private:
+    void (*m_handler_orig) (char *);
+    int m_already_prompted_orig;
 
     /* Whether the target was async.  */
-    int target_is_async_orig;
+    bool m_target_is_async_orig;
+
+    /* Processing events may change the current UI.  */
+    scoped_restore_tmpl<struct ui *> m_save_ui;
   };
 
 char *
 gdb_readline_wrapper (const char *prompt)
 {
-  struct ui *ui = current_ui;
-
-  gdb_readline_wrapper_cleanup cleanup (ui);
-
-  /* Processing events may change the current UI.  */
-  scoped_restore save_ui = make_scoped_restore (&current_ui);
-
-  if (cleanup.target_is_async_orig)
-    target_async (0);
+  /* This saves/restores global readline/gdb state around event
+     processing.  */
+  gdb_readline_wrapper_cleanup cleanup;
 
   /* Display our prompt and prevent double prompt display.  Don't pass
      down a NULL prompt, since that has special meaning for
      display_gdb_prompt -- it indicates a request to print the primary
      prompt, while we want a secondary prompt here.  */
   display_gdb_prompt (prompt != NULL ? prompt : "");
-  if (ui->command_editing)
+  if (current_ui->command_editing)
     rl_already_prompted = 1;
 
   if (after_char_processing_hook)
-- 
2.14.3

Re: [RFA 2/2] Remove cleanups from prompt_for_continue

[Pedro Alves]

On 03/23/2018 03:38 AM, Tom Tromey wrote:
> This removes the cleanups from prompt_for_continue by the use of
> unique_xmalloc_ptr.
> 
> gdb/ChangeLog
> 2018-03-22  Tom Tromey  <tom@tromey.com>
> 
> 	* utils.c (prompt_for_continue): Use unique_xmalloc_ptr.
OK.

Thanks,
Pedro Alves

Re: [RFA 1/2] Remove cleanups from gdb_readline_wrapper

[Tom Tromey]

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

Pedro> I think we can move these to the ctor too, and then all the fields
Pedro> of the structure can be made private.  Something like the patch
Pedro> below.  (Tested on x86-64 GNU/Linux.)

Seems like a good idea to me.

Pedro> While at it, since we're touching most of the structure, we might as
Pedro> well reindent it to have open { at column 0.  (Not done below to keep
Pedro> the patch readable.)

I added this and did the reindentation.
Also I removed the "explicit" from the constructor.

Tom