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

Simon Marchi
2018-06-05T15:16:23Z

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