Re: [PATCH 01/16] Change wrap buffering to use a std::string

Joel Brobecker
2018-12-23T15:26:08Z

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 <tom@tromey.com>
>
> * 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