diff mbox

[RFA,17/22] Remove make_cleanup_restore_current_uiout

Message ID 1474949330-4307-18-git-send-email-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Sept. 27, 2016, 4:08 a.m. UTC 
  This removes make_cleanup_restore_current_uiout in favor of an
RAII-based class.

2016-09-26  Tom Tromey  <tom@tromey.com>

	* ui-out.c (make_cleanup_restore_current_uiout)
	(restore_current_uiout_cleanup): Remove.
	* infrun.c (print_stop_event): Use scoped_restore_uiout.
	* ui-out.h (scoped_restore_uiout): New class.
	(make_cleanup_restore_current_uiout): Don't declare.
---
 gdb/ChangeLog       |  8 ++++++++
 gdb/infrun.c        | 15 +++++++--------
 gdb/python/python.c |  3 +--
 gdb/ui-out.c        | 18 ------------------
 gdb/ui-out.h        | 25 +++++++++++++++++++++++--
 5 files changed, 39 insertions(+), 30 deletions(-)

Comments

Trevor Saunders Sept. 29, 2016, 12:04 p.m. UTC | #1 
> +class scoped_restore_uiout
> +{
> + public:
> +
> +  scoped_restore_uiout () : saved (current_uiout)
> +  {
> +  }
> +
> +  ~scoped_restore_uiout ()
> +  {
> +    current_uiout = saved;
> +  }
> +
> + private:
> +
> +  // No need for these.  They are intentionally not defined anywhere.
> +  scoped_restore_uiout &operator= (const scoped_restore_uiout &);
> +  scoped_restore_uiout (const scoped_restore_uiout &);
> +
> +  // The saved ui out.
> +  struct ui_out *saved;

isn't this just scoped_restore<ui_out *> ? why do you need a separate
class?

Trev
Tom Tromey Sept. 29, 2016, 2:05 p.m. UTC | #2 
>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:

Trevor> isn't this just scoped_restore<ui_out *> ? why do you need a separate
Trevor> class?

In an earlier thread there was a discussion of not having this cleanup
be over-general -- that is, it was intentionally specific to just
current_uiout.

Tom
Simon Marchi Sept. 29, 2016, 5:34 p.m. UTC | #3 
On 2016-09-29 10:05, Tom Tromey wrote:
>>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> 
> Trevor> isn't this just scoped_restore<ui_out *> ? why do you need a 
> separate
> Trevor> class?
> 
> In an earlier thread there was a discussion of not having this cleanup
> be over-general -- that is, it was intentionally specific to just
> current_uiout.

In that case, may I suggest naming the class 
scoped_restore_current_uiout?

IIRC, the original comment was that it was not necessary to have a 
parametrized make_cleanup_restore_uiout, when the only uiout we restore 
is the current_ui.  And I don't think there was a cleanup to restore a 
generic pointer, so we had to have a specialized cleanup anyway.

Here we have the option to have one less class by using your 
scoped_restore:

     scoped_restore<ui_out *> uiout_restore (&current_uiout, 
other_uiout);

vs having a specific one:

     scoped_restore_current_uiout uiout_restore;
     current_uiout = other_uiout;

I am ok with both methods, it's not a big deal.

Simon
Tom Tromey Sept. 29, 2016, 5:55 p.m. UTC | #4 
Simon> Here we have the option to have one less class by using your
Simon> scoped_restore:

Simon>     scoped_restore<ui_out *> uiout_restore (&current_uiout,
Simon> other_uiout);

I don't mind if you don't mind.  I will make this change.

Tom
Pedro Alves Sept. 30, 2016, 2:14 a.m. UTC | #5 
On 09/29/2016 03:05 PM, Tom Tromey wrote:
>>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> 
> Trevor> isn't this just scoped_restore<ui_out *> ? why do you need a separate
> Trevor> class?
> 
> In an earlier thread there was a discussion of not having this cleanup
> be over-general -- that is, it was intentionally specific to just
> current_uiout.

how about:

struct scoped_restore_current_uiout : public scoped_restore<ui_out *>
{
  scoped_restore_current_uiout ()
    : scoped_restore (&current_uiout)
  {}
};

?

Thanks,
Pedro Alves
Tom Tromey Sept. 30, 2016, 9:41 p.m. UTC | #6 
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> In an earlier thread there was a discussion of not having this cleanup
>> be over-general -- that is, it was intentionally specific to just
>> current_uiout.

Pedro> how about:

Pedro> struct scoped_restore_current_uiout : public scoped_restore<ui_out *>

With the make_scoped_restore change it was simpler to just change the
few (one or two?  I forget) users to mention current_uiout explicitly.

Tom
Pedro Alves Sept. 30, 2016, 11:20 p.m. UTC | #7 
On 09/30/2016 10:41 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> In an earlier thread there was a discussion of not having this cleanup
>>> be over-general -- that is, it was intentionally specific to just
>>> current_uiout.
> 
> Pedro> how about:
> 
> Pedro> struct scoped_restore_current_uiout : public scoped_restore<ui_out *>
> 
> With the make_scoped_restore change it was simpler to just change the
> few (one or two?  I forget) users to mention current_uiout explicitly.

OK.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 770a96f..89ce46f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@ 
 2016-09-26  Tom Tromey  <tom@tromey.com>
 
+	* ui-out.c (make_cleanup_restore_current_uiout)
+	(restore_current_uiout_cleanup): Remove.
+	* infrun.c (print_stop_event): Use scoped_restore_uiout.
+	* ui-out.h (scoped_restore_uiout): New class.
+	(make_cleanup_restore_current_uiout): Don't declare.
+
+2016-09-26  Tom Tromey  <tom@tromey.com>
+
 	* elfread.c (elf_read_minimal_symbols): Use std::vector.
 
 2016-09-26  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1736526..7136d00 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8084,22 +8084,21 @@  print_stop_location (struct target_waitstatus *ws)
 void
 print_stop_event (struct ui_out *uiout)
 {
-  struct cleanup *old_chain;
   struct target_waitstatus last;
   ptid_t last_ptid;
   struct thread_info *tp;
 
   get_last_target_status (&last_ptid, &last);
 
-  old_chain = make_cleanup_restore_current_uiout ();
-  current_uiout = uiout;
-
-  print_stop_location (&last);
+  {
+    scoped_restore_uiout save_uiout;
+    current_uiout = uiout;
 
-  /* Display the auto-display expressions.  */
-  do_displays ();
+    print_stop_location (&last);
 
-  do_cleanups (old_chain);
+    /* Display the auto-display expressions.  */
+    do_displays ();
+  }
 
   tp = inferior_thread ();
   if (tp->thread_fsm != NULL
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 9e4d610..b98ef90 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -647,8 +647,7 @@  execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
       struct interp *interp;
 
       scoped_restore<int> save_async (&current_ui->async, 0);
-
-      make_cleanup_restore_current_uiout ();
+      scoped_restore_uiout save_uiout;
 
       /* Use the console interpreter uiout to have the same print format
 	for console or MI.  */
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index ec44ab6..3972a56 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -953,24 +953,6 @@  ui_out_destroy (struct ui_out *uiout)
   xfree (uiout);
 }
 
-/* Cleanup that restores a previous current uiout.  */
-
-static void
-restore_current_uiout_cleanup (void *arg)
-{
-  struct ui_out *saved_uiout = (struct ui_out *) arg;
-
-  current_uiout = saved_uiout;
-}
-
-/* See ui-out.h.  */
-
-struct cleanup *
-make_cleanup_restore_current_uiout (void)
-{
-  return make_cleanup (restore_current_uiout_cleanup, current_uiout);
-}
-
 /* Standard gdb initialization hook.  */
 
 void
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 6a4d78a..12daafd 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -247,8 +247,29 @@  extern void ui_out_destroy (struct ui_out *uiout);
 
 extern int ui_out_redirect (struct ui_out *uiout, struct ui_file *outstream);
 
-/* Make a cleanup that restores the previous current uiout.  */
+/* An RAII-based class that saves and restores the current uiout.  */
 
-extern struct cleanup *make_cleanup_restore_current_uiout (void);
+class scoped_restore_uiout
+{
+ public:
+
+  scoped_restore_uiout () : saved (current_uiout)
+  {
+  }
+
+  ~scoped_restore_uiout ()
+  {
+    current_uiout = saved;
+  }
+
+ private:
+
+  // No need for these.  They are intentionally not defined anywhere.
+  scoped_restore_uiout &operator= (const scoped_restore_uiout &);
+  scoped_restore_uiout (const scoped_restore_uiout &);
+
+  // The saved ui out.
+  struct ui_out *saved;
+};
 
 #endif /* UI_OUT_H */