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

[Joel Brobecker <brobecker@adacore.com>]

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