From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33277 invoked by alias); 25 Jan 2017 17:52:45 -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 32696 invoked by uid 89); 25 Jan 2017 17:52:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=superfluous, H*f:sk:558e1e5, stdio_file_up, that'd X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 25 Jan 2017 17:52:42 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B073F51460; Wed, 25 Jan 2017 17:52:42 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0PHqcwK013293; Wed, 25 Jan 2017 12:52:39 -0500 Subject: Re: [PATCH v2 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy To: Simon Marchi References: <1484617147-2506-1-git-send-email-palves@redhat.com> <1484617147-2506-6-git-send-email-palves@redhat.com> <5533317a1fdb2e5f36e64731aef84993@polymtl.ca> <4a6cce94-1533-36e9-6015-330e2e014a98@redhat.com> <558e1e55-23eb-716e-8117-c18d7bded7b8@redhat.com> <79c06dbccef0c33309d208316be207f4@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <8bf8c6d9-9bbb-be9b-ee5e-99c2c3fa5b8e@redhat.com> Date: Wed, 25 Jan 2017 17:52:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <79c06dbccef0c33309d208316be207f4@polymtl.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-01/txt/msg00545.txt.bz2 On 01/24/2017 06:29 PM, Simon Marchi wrote: > On 2017-01-23 18:18, Pedro Alves wrote: >> @@ -195,20 +188,18 @@ handle_redirections (int from_tty) >> return; >> } >> >> - output = gdb_fopen (logging_filename, logging_overwrite ? "w" : "a"); >> - if (output == NULL) >> + std::unique_ptr log (new stdio_file ()); > > What is the "rule" for using std::unique_ptr directly vs defining a > typedef? > > typedef std::unique_ptr stdio_file_up; I don't think there's one. I was going by "if we're going to be typing the long std::unique_ptr all other the place, add a typedef". I think that for a single use or two, requiring a typedef may be excessive? The original "rule" I proposed was about the "_up" naming. I was originally thinking of preventing a proliferation of a mix of "_uptr" vs "_up" vs "UP" vs "_whatnot" suffixes. > > I'd like if we could avoid having a mix of styles, unless there are good > reasons for it. No real good reason other than "I hadn't bothered" :-) I've added the typedef locally now. > >> + if (!log->open (logging_filename, logging_overwrite ? "w" : "a")) >> perror_with_name (_("set logging")); >> - cleanups = make_cleanup_ui_file_delete (output); >> + >> + output = log.get (); >> >> /* Redirects everything to gdb_stdout while this is running. */ >> if (!logging_redirect) >> { >> no_redirect_file = output; >> >> - output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0); >> - if (output == NULL) >> - perror_with_name (_("set logging")); >> - make_cleanup_ui_file_delete (output); >> + output = new tee_file (gdb_stdout, 0, no_redirect_file, 0); > > Is there anything that replaces this cleanup? IOW, if the > fprinf_unfiltered just below did throw an exception for some reason, > would this tee_file leak? Looks like there isn't. I'll fix it. Bah, this is going to mess up a follow-up patch to push down the tee to the interpreter callback that I had prepared meanwhile. :-) >> - return code; >> + add_code_footer (inst->scope, &buf); >> + return std::move (buf.string ()); > > I would have thought that this std::move would be superfluous, because > the compiler would do it anyway. Is it the case? Is it a good practice > to use move explicitly to make sure it's a move and not a copy (and > probably get a compile-time error if a move is not possible)? It's actually usually a bad practice, since if you do: std::string foo () { std::string str; return std::move (std); } you prevent the compiler from eliding the "str" local. In this case, "buf.string ()" is a reference, so if we don't tell the compiler to treat it as an rvalue reference, then the compiler won't know that it can move the contents out of the buffer. I.e., call the move constructor on the caller's string directly instead of the copy constructor (and doing a deep copy). >> >> -static void >> -ioscm_file_port_put (struct ui_file *file, >> - ui_file_put_method_ftype *write, >> - void *dest) >> +void >> +ioscm_file_port::flush () >> { >> - ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file); >> - >> - if (stream->magic != &file_port_magic) >> - internal_error (__FILE__, __LINE__, >> - _("ioscm_file_port_put: bad magic number")); >> - >> - /* This function doesn't meld with ports very well. */ >> } > > It looks like there's a low-level function, scm_force_output, that > "flushes" a port. I wonder if we'd want to do that in flush. Sounds like it (though I've not added it). > mi_console_file::puts looks identical to ui_file::puts. Is there a > reason to overload it? Looks like no reason. Good catch. > Throughout mi_cmd_data_read_memory, is is worth it performance-wise to > re-use the same string_file and doing some .clear() rather than > declaring different ones in more specific scopes? Since they are stack > allocated, there shouldn't be any runtime cost in execution time. Which > is faster between constructing a string_file and calling .clear()? Even though the string_buffer is stack allocated, the contents of its contents (the internal buffer inside std::string) are not. If we moved the stream local to inside the loop scope, that'd force allocating and growing the std::string's internals from scratch in each iteration. With the clear() approach, we reuse the same buffer on all iterations. "std::string::clear()" only sets the string's data size to 0, it does not reduce the string's capacity, unlike "std::string::shrink_to_fit()". > I think it would be less error-prone (like forgetting to call .clear() > between two usages) to declare multiple string_file objects in the > scopes that need them, but I understand if you'd rather not do it for > performance reasons. I also understand if you say that it's because > it's the way it worked and don't want to spend time on a deprecated > function anyway :). All the above. :-) > >> /* Expand tabs into spaces, since ncurses on MS-Windows doesn't. */ >> - ret = tui_expand_tabs (p, 0); >> + ret = tui_expand_tabs (str.c_str (), 0); > > It's not in the scope of this patch, but it would be nice to rewrite > tui_expand_tabs using C++ features, I'm sure it could be much simpler. Yeah, I'm sure it would. When I look at such things, my first thought is "where is the string going to end up stored? Would it be reasonable to cascade std::string out of the function across several layers in order to avoid the xstrdup that using std::string internally but returning char * would require?". > > > I'm not done looking at the patch yet, but I've deleted to much of the > quote and I have comments about stuff I deleted, so it will be simpler > to just send another e-mail. Thanks, Pedro Alves