Re: [PATCH] GDB PR tdep/8282: MIPS: Wire in `set disassembler-options'

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Maciej,

I just had a few minutes to look at it, so I looked at the API in 
dis-asm.h, I have one small comment.

> Index: gdb/include/dis-asm.h
> ===================================================================
> --- gdb.orig/include/dis-asm.h	2018-06-04 16:53:56.536062303 +0100
> +++ gdb/include/dis-asm.h	2018-06-05 00:20:26.008674269 +0100
> @@ -222,16 +222,50 @@ typedef struct disassemble_info
> 
>  } disassemble_info;
> 
> -/* This struct is used to pass information about valid disassembler 
> options
> -   and their descriptions from the target to the generic GDB functions 
> that
> -   set and display them.  */
> +/* This struct is used to pass information about valid disassembler
> +   option arguments from the target to the generic GDB functions
> +   that set and display them.  */
> +
> +typedef struct
> +{
> +  /* Option argument name to use in descriptions.  */
> +  const char *name;
> +
> +  /* Vector of acceptable option argument values, NULL-terminated.  */
> +  const char **values;
> +} disasm_option_arg_t;
> +
> +/* This struct is used to pass information about valid disassembler
> +   options, their descriptions and arguments from the target to the
> +   generic GDB functions that set and display them.  Options are
> +   defined by tuples of vector entries at each index.  */
> 
>  typedef struct
>  {
> +  /* Vector of option names, NULL-terminated.  */
>    const char **name;
> +
> +  /* Vector of option descriptions or NULL if none to be shown.  */
>    const char **description;
> +
> +  /* Vector of option argument information pointers or NULL if no
> +     option accepts an argument.  NULL entries denote individual
> +     options that accept no argument.  */
> +  const disasm_option_arg_t **arg;
>  } disasm_options_t;
> 
> +/* This struct is used to pass information about valid disassembler
> +   options and arguments from the target to the generic GDB functions
> +   that set and display them.  */
> +
> +typedef struct
> +{
> +  /* Valid disassembler options.  */
> +  disasm_options_t options;
> +
> +  /* Vector of acceptable option arguments, NULL-terminated.  */
> +  disasm_option_arg_t *args;
> +} disasm_options_and_args_t;

It took me a bit of time why we have a vector of disasm_option_arg_t 
here and in disasm_options_t.  I finally understood that 
disasm_options_and_args_t::args owns the disasm_option_arg_t objects, 
and the pointers in disasm_options_t::arg point to these instances.  
Maybe that could be clarified in both comments?

Do we really need a new struct, or could the "args" field be part of the 
disasm_options_t struct directly?

Simon