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

Simon Marchi
2016-10-01T04:28:24Z

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