diff mbox

[RFA,03/22] Use scoped_restore for ui_file

Message ID 1474949330-4307-4-git-send-email-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey Sept. 27, 2016, 4:08 a.m. UTC
This replaces all the uses of make_cleanup_restore_ui_file with
scoped_restore.

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

	* utils.c (make_cleanup_restore_ui_file, do_restore_ui_file)
	(struct restore_ui_file_closure): Remove.
	* utils.h (make_cleanup_restore_ui_file): Don't declare.
	* guile/scm-ports.c (ioscm_with_output_to_port_worker): Use
	scoped_restore.
	* top.c (execute_command_to_string): Use scoped_restore.
---
 gdb/ChangeLog         |  9 +++++++++
 gdb/guile/scm-ports.c | 10 ++++------
 gdb/top.c             | 15 +++++----------
 gdb/utils.c           | 29 -----------------------------
 gdb/utils.h           |  3 ---
 5 files changed, 18 insertions(+), 48 deletions(-)

Comments

Simon Marchi Oct. 1, 2016, 4:28 a.m. UTC | #1
On 2016-09-27 00:08, Tom Tromey wrote:
> This replaces all the uses of make_cleanup_restore_ui_file with
> scoped_restore.
> 
> 2016-09-26  Tom Tromey  <tom@tromey.com>
> 
> 	* utils.c (make_cleanup_restore_ui_file, do_restore_ui_file)
> 	(struct restore_ui_file_closure): Remove.
> 	* utils.h (make_cleanup_restore_ui_file): Don't declare.
> 	* guile/scm-ports.c (ioscm_with_output_to_port_worker): Use
> 	scoped_restore.
> 	* top.c (execute_command_to_string): Use scoped_restore.
> ---
>  gdb/ChangeLog         |  9 +++++++++
>  gdb/guile/scm-ports.c | 10 ++++------
>  gdb/top.c             | 15 +++++----------
>  gdb/utils.c           | 29 -----------------------------
>  gdb/utils.h           |  3 ---
>  5 files changed, 18 insertions(+), 48 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 104048f..da69ce8 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,14 @@
>  2016-09-26  Tom Tromey  <tom@tromey.com>
> 
> +	* utils.c (make_cleanup_restore_ui_file, do_restore_ui_file)
> +	(struct restore_ui_file_closure): Remove.
> +	* utils.h (make_cleanup_restore_ui_file): Don't declare.
> +	* guile/scm-ports.c (ioscm_with_output_to_port_worker): Use
> +	scoped_restore.
> +	* top.c (execute_command_to_string): Use scoped_restore.
> +
> +2016-09-26  Tom Tromey  <tom@tromey.com>
> +
>  	* utils.h (class scoped_restore): New class.
>  	* top.c (execute_command_to_string): Use scoped_restore.
>  	* python/python.c (python_interactive_command): Use
> diff --git a/gdb/guile/scm-ports.c b/gdb/guile/scm-ports.c
> index 5559475..96e4372 100644
> --- a/gdb/guile/scm-ports.c
> +++ b/gdb/guile/scm-ports.c
> @@ -524,15 +524,13 @@ ioscm_with_output_to_port_worker (SCM port, SCM
> thunk, enum oport oport,
> 
>    make_cleanup_ui_file_delete (port_file);
> 
> +  scoped_restore<ui_file *> save_file (oport == GDB_STDERR
> +				       ? &gdb_stderr : &gdb_stdout);
> +
>    if (oport == GDB_STDERR)
> -    {
> -      make_cleanup_restore_ui_file (&gdb_stderr);
> -      gdb_stderr = port_file;
> -    }
> +    gdb_stderr = port_file;
>    else
>      {
> -      make_cleanup_restore_ui_file (&gdb_stdout);
> -

I think that situations like this, where cleanups are created in an 
scope inner to the function scope, but ran at the end of the function 
scope, are quite frequent in gdb.  You obviously can't simply define the 
scoped_restore in the inner scope, as it wouldn't have the desired 
effect.  I think that it could be worked around using the general 
pattern:

   shared_ptr< scoped_restore<ui_file *> > save_file;

   if (...)
     {
       save_file = new scoped_restore<ui_file *>(&gdb_stderr, port_file)
     }
   else
     {
       save_file = new scoped_restore<ui_file *>(&gdb_stdout, port_file)
     }

We can't use std::shared_ptr, since it's only in c++11, but I think it's 
just a matter of time before we define our own version of it.

An alternative would be to have a default constructor for 
scoped_restore, that creates an inactive scoped_restore, and then assign 
it a variable to restore later with acquire(T* var)/acquire(T* var, T 
value) methods or something.  I am not sure which one is better.

To support some use cases where discard_cleanups is used, we might need 
a way to "release" the scoped_restore, which would essentially cancel 
it.  So if we have a "release" method, maybe having the symmetrical 
"acquire" would make sense.

What do you think?

Simon
Tom Tromey Oct. 1, 2016, 5:22 a.m. UTC | #2
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon>   shared_ptr< scoped_restore<ui_file *> > save_file;

Why shared_ptr and not unique_ptr?

Simon> We can't use std::shared_ptr, since it's only in c++11, but I think
Simon> it's just a matter of time before we define our own version of it.

... Pedro's branch has unique_ptr :)

Simon> An alternative would be to have a default constructor for
Simon> scoped_restore, that creates an inactive scoped_restore, and then
Simon> assign it a variable to restore later with acquire(T* var)/acquire(T*
Simon> var, T value) methods or something.  I am not sure which one is
Simon> better.

FWIW Mozilla uses an Option class for this kind of thing.
It works like:

   Option< scoped_restore<ui_file *> > save_file;
   if (mumble) {
     save_file.emplace (make_scoped_restore (&var));
   }

The main advantage of this over *_ptr is that Option contains the
object, so no heap allocations are required.

Simon> To support some use cases where discard_cleanups is used, we might
Simon> need a way to "release" the scoped_restore, which would essentially
Simon> cancel it.  So if we have a "release" method, maybe having the
Simon> symmetrical "acquire" would make sense.

Simon> What do you think?

It seems sensible to me.

One idea to consider is whether it's better to have a separate
"discardable" variant of scoped_restore (or whatever else); the idea
being that then the type name serves as a signal to look more closely at
the logic.  I think most spots don't need discardable cleanups.

Tom
Simon Marchi Oct. 1, 2016, 11:47 a.m. UTC | #3
On 2016-10-01 01:22, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon>   shared_ptr< scoped_restore<ui_file *> > save_file;
> 
> Why shared_ptr and not unique_ptr?

Right, I am not used yet to think about their differences, and which one 
applies to a particular situation.  unique_ptr is clearler a better 
choice.

> Simon> We can't use std::shared_ptr, since it's only in c++11, but I 
> think
> Simon> it's just a matter of time before we define our own version of 
> it.
> 
> ... Pedro's branch has unique_ptr :)

Oh, right!

> Simon> An alternative would be to have a default constructor for
> Simon> scoped_restore, that creates an inactive scoped_restore, and 
> then
> Simon> assign it a variable to restore later with acquire(T* 
> var)/acquire(T*
> Simon> var, T value) methods or something.  I am not sure which one is
> Simon> better.
> 
> FWIW Mozilla uses an Option class for this kind of thing.
> It works like:
> 
>    Option< scoped_restore<ui_file *> > save_file;
>    if (mumble) {
>      save_file.emplace (make_scoped_restore (&var));
>    }
> 
> The main advantage of this over *_ptr is that Option contains the
> object, so no heap allocations are required.

That sounds good, it ends up with the same behavior, but implemented 
better.

> Simon> To support some use cases where discard_cleanups is used, we 
> might
> Simon> need a way to "release" the scoped_restore, which would 
> essentially
> Simon> cancel it.  So if we have a "release" method, maybe having the
> Simon> symmetrical "acquire" would make sense.
> 
> Simon> What do you think?
> 
> It seems sensible to me.
> 
> One idea to consider is whether it's better to have a separate
> "discardable" variant of scoped_restore (or whatever else); the idea
> being that then the type name serves as a signal to look more closely 
> at
> the logic.  I think most spots don't need discardable cleanups.

Yep, alsop sounds good.
Tom Tromey Oct. 13, 2016, 2:56 p.m. UTC | #4
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> FWIW Mozilla uses an Option class for this kind of thing.
>> It works like:

If you happened to search Firefox looking for this, it's called Maybe,
not Option -- Option is the Rust name for this.

Also I thought it was worth pointing out that this has made it into
C++17 under the name std::optional:

http://en.cppreference.com/w/cpp/utility/optional

Tom
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 104048f..da69ce8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@ 
 2016-09-26  Tom Tromey  <tom@tromey.com>
 
+	* utils.c (make_cleanup_restore_ui_file, do_restore_ui_file)
+	(struct restore_ui_file_closure): Remove.
+	* utils.h (make_cleanup_restore_ui_file): Don't declare.
+	* guile/scm-ports.c (ioscm_with_output_to_port_worker): Use
+	scoped_restore.
+	* top.c (execute_command_to_string): Use scoped_restore.
+
+2016-09-26  Tom Tromey  <tom@tromey.com>
+
 	* utils.h (class scoped_restore): New class.
 	* top.c (execute_command_to_string): Use scoped_restore.
 	* python/python.c (python_interactive_command): Use
diff --git a/gdb/guile/scm-ports.c b/gdb/guile/scm-ports.c
index 5559475..96e4372 100644
--- a/gdb/guile/scm-ports.c
+++ b/gdb/guile/scm-ports.c
@@ -524,15 +524,13 @@  ioscm_with_output_to_port_worker (SCM port, SCM thunk, enum oport oport,
 
   make_cleanup_ui_file_delete (port_file);
 
+  scoped_restore<ui_file *> save_file (oport == GDB_STDERR
+				       ? &gdb_stderr : &gdb_stdout);
+
   if (oport == GDB_STDERR)
-    {
-      make_cleanup_restore_ui_file (&gdb_stderr);
-      gdb_stderr = port_file;
-    }
+    gdb_stderr = port_file;
   else
     {
-      make_cleanup_restore_ui_file (&gdb_stdout);
-
       if (ui_out_redirect (current_uiout, port_file) < 0)
 	warning (_("Current output protocol does not support redirection"));
       else
diff --git a/gdb/top.c b/gdb/top.c
index 84285b2..d782466 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -706,22 +706,17 @@  execute_command_to_string (char *p, int from_tty)
   str_file = mem_fileopen ();
 
   make_cleanup_ui_file_delete (str_file);
-  make_cleanup_restore_ui_file (&gdb_stdout);
-  make_cleanup_restore_ui_file (&gdb_stderr);
-  make_cleanup_restore_ui_file (&gdb_stdlog);
-  make_cleanup_restore_ui_file (&gdb_stdtarg);
-  make_cleanup_restore_ui_file (&gdb_stdtargerr);
 
   if (ui_out_redirect (current_uiout, str_file) < 0)
     warning (_("Current output protocol does not support redirection"));
   else
     make_cleanup_ui_out_redirect_pop (current_uiout);
 
-  gdb_stdout = str_file;
-  gdb_stderr = str_file;
-  gdb_stdlog = str_file;
-  gdb_stdtarg = str_file;
-  gdb_stdtargerr = str_file;
+  scoped_restore<struct ui_file *> save_stdout (&gdb_stdout, str_file);
+  scoped_restore<struct ui_file *> save_stderr (&gdb_stderr, str_file);
+  scoped_restore<struct ui_file *> save_stdlog (&gdb_stdlog, str_file);
+  scoped_restore<struct ui_file *> save_stdtarg (&gdb_stdtarg, str_file);
+  scoped_restore<struct ui_file *> save_stdtargerr (&gdb_stdtargerr, str_file);
 
   execute_command (p, from_tty);
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 9a83053..5fb4b28 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -319,35 +319,6 @@  make_cleanup_htab_delete (htab_t htab)
   return make_cleanup (do_htab_delete_cleanup, htab);
 }
 
-struct restore_ui_file_closure
-{
-  struct ui_file **variable;
-  struct ui_file *value;
-};
-
-static void
-do_restore_ui_file (void *p)
-{
-  struct restore_ui_file_closure *closure
-    = (struct restore_ui_file_closure *) p;
-
-  *(closure->variable) = closure->value;
-}
-
-/* Remember the current value of *VARIABLE and make it restored when
-   the cleanup is run.  */
-
-struct cleanup *
-make_cleanup_restore_ui_file (struct ui_file **variable)
-{
-  struct restore_ui_file_closure *c = XNEW (struct restore_ui_file_closure);
-
-  c->variable = variable;
-  c->value = *variable;
-
-  return make_cleanup_dtor (do_restore_ui_file, (void *) c, xfree);
-}
-
 /* Helper for make_cleanup_value_free_to_mark.  */
 
 static void
diff --git a/gdb/utils.h b/gdb/utils.h
index 63583ed..6179582 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -134,9 +134,6 @@  extern struct cleanup *make_cleanup_restore_uinteger (unsigned int *variable);
 struct target_ops;
 extern struct cleanup *make_cleanup_unpush_target (struct target_ops *ops);
 
-extern struct cleanup *
-  make_cleanup_restore_ui_file (struct ui_file **variable);
-
 extern struct cleanup *make_cleanup_value_free_to_mark (struct value *);
 extern struct cleanup *make_cleanup_value_free (struct value *);