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

["Maciej W. Rozycki" <macro at mips dot com>]

Hi Simon,

> 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?

 Sure, I can add some explanatory notes.  I'll collect comments other 
people may make an post an update then.

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

 Possibly.  I thought of a notational separation between the two kinds of 
objects.  I'm not sure if there is a clear advantage either way though, 
so I'm inclined to leave it as I wrote it.  Also having `args' and `arg' 
both as members of the same structure is surely asking for confusion.

 Actually I think `disasm_options_t' could instead be redefined as:

typedef struct
{
  const char *name;
  const char *description;
  const disasm_option_arg_t *arg;
} disasm_options_t;

and then:

typedef struct
{
  disasm_options_t *options;
  disasm_option_arg_t *args;
} disasm_options_and_args_t;

so that static initialisers could be reasonably used rather than current 
run-time setup, at least for the `options' part.  The current solution has 
the advantage of saving a small amount of memory when no descriptions (and 
now also no arguments) are used.  However I think this is all lost with 
the extra code required for run-time setup.  I propose to consider this a 
separate matter though.  Perhaps we could keep the current data structures
as they stand and write static initialisers with the aid of some macros 
(so that option names are bundled with the respective descriptions and 
arguments).

 Thanks for your review.

  Maciej