Re: [PATCH 1/6] New function null_stream

Luis Machado
2017-01-17T13:49:24Z

On 01/16/2017 04:02 AM, Yao Qi wrote:
> This patch adds a new function null_stream, which returns a null
> stream. The null stream can be used in multiple places. It is
> used in gdb_insn_length, and the following patches will use it too.
>
> gdb:
>
> 2017-01-13 Yao Qi <yao.qi@linaro.org>
>
> * disasm.c (do_ui_file_delete): Delete.
> (gdb_insn_length): Move code creating stream to ...
> * utils.c (null_stream): ... here. New function.
> * utils.h (null_stream): Declare.
> ---
> gdb/disasm.c | 17 +----------------
> gdb/utils.c | 13 +++++++++++++
> gdb/utils.h | 4 ++++
> 3 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index f419501..ae3a2f1 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -838,28 +838,13 @@ gdb_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
> return length;
> }
>
> -static void
> -do_ui_file_delete (void *arg)
> -{
> - ui_file_delete ((struct ui_file *) arg);
> -}
> -
> /* Return the length in bytes of the instruction at address MEMADDR in
> debugged memory. */
>
> int
> gdb_insn_length (struct gdbarch *gdbarch, CORE_ADDR addr)
> {
> - static struct ui_file *null_stream = NULL;
> -
> - /* Dummy file descriptor for the disassembler. */
> - if (!null_stream)
> - {
> - null_stream = ui_file_new ();
> - make_final_cleanup (do_ui_file_delete, null_stream);
> - }
> -
> - return gdb_print_insn (gdbarch, addr, null_stream, NULL);
> + return gdb_print_insn (gdbarch, addr, null_stream (), NULL);
> }
>
> /* fprintf-function for gdb_buffered_insn_length. This function is a
> diff --git a/gdb/utils.c b/gdb/utils.c
> index f142ffe..7bad193 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -199,6 +199,19 @@ make_cleanup_ui_file_delete (struct ui_file *arg)
> return make_cleanup (do_ui_file_delete, arg);
> }
>
> +struct ui_file *
> +null_stream (void)
> +{
> + static struct ui_file *stream = NULL;
> +
> + if (stream == NULL)
> + {
> + stream = ui_file_new ();
> + make_final_cleanup (do_ui_file_delete, stream);
> + }

Since we're explicitly setting stream to NULL, we will always execute
the conditional block, so it is not needed. Unless this is supposed to
reuse a stream, but in that case the code would be incorrect.

> + return stream;
> +}
> +
> /* Helper function for make_cleanup_ui_out_redirect_pop. */
>
> static void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index c548a50..26e60af 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -189,6 +189,10 @@ extern struct ui_file *gdb_stdtarg;
> extern struct ui_file *gdb_stdtargerr;
> extern struct ui_file *gdb_stdtargin;
>
> +/* Return a null stream. */
> +

Spurious newline between comment and prototype.

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Empty_line_between_subprogram_description_and_the_subprogram_implementation

> +extern struct ui_file *null_stream (void);
> +
> /* Set the screen dimensions to WIDTH and HEIGHT. */
>
> extern void set_screen_width_and_height (int width, int height);
>

Otherwise looks fine.