From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17001 invoked by alias); 23 Dec 2018 15:26:08 -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 16992 invoked by uid 89); 23 Dec 2018 15:26:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=indicating, window, entertained, malloc'd X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 23 Dec 2018 15:26:06 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id BAE101170A3; Sun, 23 Dec 2018 10:26:04 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id qsVu7-8u9Yij; Sun, 23 Dec 2018 10:26:04 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 430E11170A2; Sun, 23 Dec 2018 10:26:04 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 8E99C86763; Sun, 23 Dec 2018 19:25:58 +0400 (+04) Date: Sun, 23 Dec 2018 15:26:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 01/16] Change wrap buffering to use a std::string Message-ID: <20181223152558.GE5246@adacore.com> References: <20181128001435.12703-1-tom@tromey.com> <20181128001435.12703-2-tom@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181128001435.12703-2-tom@tromey.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2018-12/txt/msg00285.txt.bz2 Hi Tom, > Currently wrap buffering is implemented by allocating a string that is > the same width as the window, and then writing characters into it. > However, if gdb emits terminal escapes, then these could possibly > overflow the buffer. > > To prevent this, change the wrap buffer to be a std::string and update > the various uses. > > This also changes utils.c to always emit characters to the wrap > buffer. This simplifies future patches which emit terminal escape > sequences, and also makes it possible for the "echo" and "printf" > commands to be used to emit terminal escapes and have these work in > the TUI. > > gdb/ChangeLog > 2018-11-27 Tom Tromey > > * utils.c (filter_initalized): New global. > (wrap_buffer): Now a std::string. > (wrap_pointer): Remove. > (flush_wrap_buffer): New function. > (filtered_printing_initialized, set_width, wrap_here) > (fputs_maybe_filtered): Update. This patch kept me entertained for quite a bit longer than I thought! I think just the mechanis of replacing the manually-managed buffer with a string is a win in and of itself. The question your patch raised as a side-effect is whether we still need the boolean indicating whether the wrap buffer has been initialized or not (filter_initalized). I tried to explore that question, hence the unexpected entertainment, and the answer is still not clear to me, but I *think* that we should be able to. It really depends what the semantics of filtered_printing_initialized is, and it looks like it might be used for two purposes, rather than one. Eg: one meaning is the wrap_buffer is indeed iniatilized and therefore can be used; but it looks like the other use is to determine whether buffering is done at all. Anyways, this is orthogonal to your patch, so I will stop rambling now. Overall, I think it is a good patch. I have one comment and one question. > --- > gdb/ChangeLog | 9 ++++++++ > gdb/utils.c | 63 +++++++++++++++++++++------------------------------ > 2 files changed, 35 insertions(+), 37 deletions(-) > > diff --git a/gdb/utils.c b/gdb/utils.c > index 0577e64ea8..0f1953a66d 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -1268,13 +1268,11 @@ static bool pagination_disabled_for_command; > the end of the line, we spit out a newline, the indent, and then > the buffered output. */ > > -/* Malloc'd buffer with chars_per_line+2 bytes. Contains characters which > - are waiting to be output (they have already been counted in chars_printed). > - When wrap_buffer[0] is null, the buffer is empty. */ > -static char *wrap_buffer; > +static bool filter_initalized = false; Small typo in the name of that static global "filter_initalized" -> "filter_initialized" :-) > @@ -1669,6 +1666,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, > || top_level_interpreter () == NULL > || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ()) > { > + flush_wrap_buffer (stream); > fputs_unfiltered (linebuffer, stream); > return; > } I understand why this is needed today; but I can't determine whether this is needed as a consequence of your patch, or if this was a hole in the previous code. Can you explain what made you make this change? Thank you, -- Joel