[RFA,03/22] Use scoped_restore for ui_file

  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(-)
  

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
  

>>>>> "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
  

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.
  

>>>>> "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