This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]