Re: [RFA 03/22] Use scoped_restore for ui_file

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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