From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116925 invoked by alias); 1 Oct 2016 04:28:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 116914 invoked by uid 89); 1 Oct 2016 04:28:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=cleanups, thunk, H*r:112, HX-Envelope-From:sk:simon.m X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 01 Oct 2016 04:28:13 +0000 Received: by simark.ca (Postfix, from userid 112) id 569E21E130; Sat, 1 Oct 2016 00:28:11 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 7CE3C1E109; Sat, 1 Oct 2016 00:28:10 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 01 Oct 2016 04:28:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA 03/22] Use scoped_restore for ui_file In-Reply-To: <1474949330-4307-4-git-send-email-tom@tromey.com> References: <1474949330-4307-1-git-send-email-tom@tromey.com> <1474949330-4307-4-git-send-email-tom@tromey.com> Message-ID: <7417a71f254c7f42026408c5e143b374@simark.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.0 X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg00003.txt.bz2 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 > > * 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 > > + * 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 > + > * 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 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 > save_file; if (...) { save_file = new scoped_restore(&gdb_stderr, port_file) } else { save_file = new scoped_restore(&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