Re: [PATCH 1/6] New function null_stream

[Luis Machado <lgustavo@codesourcery.com>]

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.