[RFA,03/22] Use scoped_restore for ui_file
Commit Message
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
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
@@ -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
@@ -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
@@ -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);
@@ -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
@@ -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 *);