RE: Re: [Patch] Try2: -var-evaluate-expression [-f FORMAT] NAME

["Marc Khouzam" <marc dot khouzam at ericsson dot com>]

> Â Â Â Â * varobj.h (varobj_get_formatted_value): Declare.
> > Â Â Â Â * varobj.c (my_value_of_variable): Added format parameter.
> > Â Â Â Â (cplus_value_of_variable): Likewise.
> > Â Â Â Â (java_value_of_variable): Likewise.
> > Â Â Â Â (*(value_of_variable)): Likewise.
> 
> Is '*(value_of_variable)' really a name of a function? :-)

After Daniel's comment, I'm not too sure what you guys do 
for function pointers that are members.... 
so I removed this Changelog entry. :-)

> > +/* Parse a string argument into a format value. Â*/
> 
> As meta-remark, can you pass the "-p" option to diff when generating patches,
> so that function names are present right in the patch?


Sounds good but I'm not sure how to tell eclipse to do that...
I'll keep looking, but until then, please forgive my patch since I ran
it without

> ..and make mi_parse_format emit an error message both if the passed format
> is NULL and when it's not NULL, but unknown?

Yes, that is nicer.  Done.


> > + Â Â Â case OP_FORMAT:
> > + Â Â Â Â if (formatFound)
> > + Â Â Â Â Â error (_("mi_cmd_var_evaluate_expression: cannot specify format more than once"));
> > + Â 
> 
> I think the current position is that error messages need not name the name of function, since
> if this error message ever make it to the user, he has no idea what
> is "mi_cmd_var_evaluate_expression", and when debugging GDB or frontend, it's not very
> helpful. Can you drop that prefix, here, and everywhere?

Done, but only in this method.  Other methods should be cleaned up in a separate patch, right?

> > + Âif (formatFound)
> > + Â Âui_out_field_string (uiout, "value", varobj_get_formatted_value (var, format));
> 
> It's not the problem introduced by your patch, but still -- it appears we have a memory
> leak here. In c_value_of_variable, we either xstrdup the value, or use value_get_print_value
> which uses ui_file_xstrdup, so we end up with newly allocated char* pointer. I think
> we have to free it here.

Good eye!
But I think you found a bigger problem, as this seems to be the
case for may other call to varobj.c.  For example, I believe there is also a leak in when
calling and not freeing:
  varobj_get_expression
  varobj_get_type
  ...

How do you suggest we handle this?  I can make a patch that attempts to fix all these leaks...
Until then, I left things unchanged everywhere.

> > + Âelse
> > + Â Âui_out_field_string (uiout, "value", varobj_get_value (var));
> > Â
> > - Âui_out_field_string (uiout, "value", varobj_get_value (var));
> > Â Âreturn MI_CMD_DONE;
> 
> The code patch is OK with those changes, thanks!
> Eli, does the doc patch look good for you?

Thanks, below is the revised patch.  I'll wait for your and Eli's ok.

Marc

==

gdb/ChangeLog:
2008-04-02  Marc Khouzam  <marc.khouzam@ericsson.com>
 
        * mi/mi-cmd-var.c: Include "mi-getopt.h".
        (mi_parse_format): New.  Factored out from mi_cmd_var_set_format.
        (mi_cmd_var_set_format): Use new mi_parse_format.
        (mi_cmd_var_evaluate_expression): Support for -f option to specify
        format.
        * Makefile.in (mi-cmd-var.o): Update dependencies.

        * varobj.h (varobj_get_formatted_value): Declare.
        * varobj.c (my_value_of_variable): Added format parameter.
        (cplus_value_of_variable): Likewise.
        (java_value_of_variable): Likewise.
        (*(value_of_variable)): Likewise.
        (c_value_of_variable): Likewise.  Evaluate expression based
        on format parameter.
        (varobj_get_formatted_value): New.
        (varobj_get_value): Added format parameter to method call.

gdb/doc/ChangeLog:
2008-04-02  Marc Khouzam  <marc.khouzam@ericsson.com>

        * gdb.texinfo (GDB/MI Variable Objects): Add anchor to 
        -var-set-format.  Add -f option to -var-evaluate-expression.

gdb/testsuite/ChangeLog:
2008-04-02  Marc Khouzam  <marc.khouzam@ericsson.com>
 
        * gdb.mi/mi2-var-display.exp: Added tests for the new -f
        option of -var-evaluate-expression.
        * gdb.mi/mi2-var-display.exp: Likewise.


Index: gdb/testsuite/gdb.mi/mi-var-display.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-display.exp,v
retrieving revision 1.24
diff -u -r1.24 mi-var-display.exp
--- gdb/testsuite/gdb.mi/mi-var-display.exp     1 Apr 2008 15:18:30 -0000       1.24
+++ gdb/testsuite/gdb.mi/mi-var-display.exp     2 Apr 2008 18:42:34 -0000
@@ -168,6 +168,50 @@
        "\\^done,value=\"3\"" \
        "eval variable foo"
 
+
+# Test: c_variable-6.19
+# Desc: check optional format parameter of var-evaluate-expression
+#       and check that current format is not changed
+mi_gdb_test "-var-evaluate-expression -f hex foo" \
+       "\\^done,value=\"0x3\"" \
+       "eval variable foo in hex"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in hex"
+
+mi_gdb_test "-var-evaluate-expression -f octal foo" \
+       "\\^done,value=\"03\"" \
+       "eval variable foo in octal"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in octal"
+
+mi_gdb_test "-var-evaluate-expression -f decimal foo" \
+       "\\^done,value=\"3\"" \
+       "eval variable foo in decimal"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in decimal"
+
+mi_gdb_test "-var-evaluate-expression -f nat foo" \
+       "\\^done,value=\"0x3\"" \
+       "eval variable foo in natural"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in natural"
+
+mi_gdb_test "-var-evaluate-expression -f bin foo" \
+       "\\^done,value=\"11\"" \
+       "eval variable foo in binary"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in binary"
+
 mi_gdb_test "-var-delete foo" \
        "\\^done,ndeleted=\"1\"" \
        "delete var foo"
Index: gdb/testsuite/gdb.mi/mi2-var-display.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi2-var-display.exp,v
retrieving revision 1.17
diff -u -r1.17 mi2-var-display.exp
--- gdb/testsuite/gdb.mi/mi2-var-display.exp    1 Apr 2008 15:18:30 -0000       1.17
+++ gdb/testsuite/gdb.mi/mi2-var-display.exp    2 Apr 2008 18:42:34 -0000
@@ -168,6 +168,49 @@
        "\\^done,value=\"3\"" \
        "eval variable foo"
 
+# Test: c_variable-6.19
+# Desc: check optional format parameter of var-evaluate-expression
+#       and check that current format is not changed
+mi_gdb_test "-var-evaluate-expression -f hex foo" \
+       "\\^done,value=\"0x3\"" \
+       "eval variable foo in hex"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in hex"
+
+mi_gdb_test "-var-evaluate-expression -f octal foo" \
+       "\\^done,value=\"03\"" \
+       "eval variable foo in octal"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in octal"
+
+mi_gdb_test "-var-evaluate-expression -f decimal foo" \
+       "\\^done,value=\"3\"" \
+       "eval variable foo in decimal"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in decimal"
+
+mi_gdb_test "-var-evaluate-expression -f nat foo" \
+       "\\^done,value=\"0x3\"" \
+       "eval variable foo in natural"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in natural"
+
+mi_gdb_test "-var-evaluate-expression -f bin foo" \
+       "\\^done,value=\"11\"" \
+       "eval variable foo in binary"
+
+mi_gdb_test "-var-show-format foo" \
+       "\\^done,format=\"decimal\"" \
+       "show format variable foo after eval in binary"
+
 mi_gdb_test "-var-delete foo" \
        "\\^done,ndeleted=\"1\"" \
        "delete var foo"
Index: gdb/varobj.h
===================================================================
RCS file: /cvs/src/src/gdb/varobj.h,v
retrieving revision 1.17
diff -u -r1.17 varobj.h
--- gdb/varobj.h        26 Mar 2008 14:51:28 -0000      1.17
+++ gdb/varobj.h        2 Apr 2008 18:42:28 -0000
@@ -111,6 +111,9 @@
 
 extern int varobj_get_attributes (struct varobj *var);
 
+extern char *varobj_get_formatted_value (struct varobj *var,
+                                        enum varobj_display_formats format);
+
 extern char *varobj_get_value (struct varobj *var);
 
 extern int varobj_set_value (struct varobj *var, char *expression);
Index: gdb/varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.108
diff -u -r1.108 varobj.c
--- gdb/varobj.c        26 Mar 2008 14:51:28 -0000      1.108
+++ gdb/varobj.c        2 Apr 2008 18:42:28 -0000
@@ -228,7 +228,8 @@
 
 static struct value *value_of_child (struct varobj *parent, int index);
 
-static char *my_value_of_variable (struct varobj *var);
+static char *my_value_of_variable (struct varobj *var,
+                                  enum varobj_display_formats format);
 
 static char *value_get_print_value (struct value *value,
                                    enum varobj_display_formats format);
@@ -253,7 +254,8 @@
 
 static struct type *c_type_of_child (struct varobj *parent, int index);
 
-static char *c_value_of_variable (struct varobj *var);
+static char *c_value_of_variable (struct varobj *var,
+                                 enum varobj_display_formats format);
 
 /* C++ implementation */
 
@@ -273,7 +275,8 @@
 
 static struct type *cplus_type_of_child (struct varobj *parent, int index);
 
-static char *cplus_value_of_variable (struct varobj *var);
+static char *cplus_value_of_variable (struct varobj *var,
+                                     enum varobj_display_formats format);
 
 /* Java implementation */
 
@@ -291,7 +294,8 @@
 
 static struct type *java_type_of_child (struct varobj *parent, int index);
 
-static char *java_value_of_variable (struct varobj *var);
+static char *java_value_of_variable (struct varobj *var,
+                                    enum varobj_display_formats format);
 
 /* The language specific vector */
 
@@ -324,7 +328,8 @@
   struct type *(*type_of_child) (struct varobj * parent, int index);
 
   /* The current value of VAR. */
-  char *(*value_of_variable) (struct varobj * var);
+  char *(*value_of_variable) (struct varobj * var,
+                             enum varobj_display_formats format);
 };
 
 /* Array of known source language routines. */
@@ -858,9 +863,16 @@
 }
 
 char *
+varobj_get_formatted_value (struct varobj *var,
+                           enum varobj_display_formats format)
+{
+  return my_value_of_variable (var, format);
+}
+
+char *
 varobj_get_value (struct varobj *var)
 {
-  return my_value_of_variable (var);
+  return my_value_of_variable (var, var->format);
 }
 
 /* Set the value of an object variable (if it is editable) to the
@@ -1777,10 +1789,10 @@
 
 /* GDB already has a command called "value_of_variable". Sigh. */
 static char *
-my_value_of_variable (struct varobj *var)
+my_value_of_variable (struct varobj *var, enum varobj_display_formats format)
 {
   if (var->root->is_valid)
-    return (*var->root->lang->value_of_variable) (var);
+    return (*var->root->lang->value_of_variable) (var, format);
   else
     return NULL;
 }
@@ -2253,7 +2265,7 @@
 }
 
 static char *
-c_value_of_variable (struct varobj *var)
+c_value_of_variable (struct varobj *var, enum varobj_display_formats format)
 {
   /* BOGUS: if val_print sees a struct/class, or a reference to one,
      it will print out its children instead of "{...}".  So we need to
@@ -2298,7 +2310,13 @@
 
            gdb_assert (varobj_value_is_changeable_p (var));
            gdb_assert (!value_lazy (var->value));
-           return xstrdup (var->print_value);
+           
+           /* If the specified format is the current one,
+              we can reuse print_value */
+           if (format == var->format)
+             return xstrdup (var->print_value);
+           else
+             return value_get_print_value (var->value, format);
          }
       }
     }
@@ -2624,7 +2642,7 @@
 }
 
 static char *
-cplus_value_of_variable (struct varobj *var)
+cplus_value_of_variable (struct varobj *var, enum varobj_display_formats format)
 {
 
   /* If we have one of our special types, don't print out
@@ -2632,7 +2650,7 @@
   if (CPLUS_FAKE_CHILD (var))
     return xstrdup ("");
 
-  return c_value_of_variable (var);
+  return c_value_of_variable (var, format);
 }
 

 /* Java */
@@ -2707,9 +2725,9 @@
 }
 
 static char *
-java_value_of_variable (struct varobj *var)
+java_value_of_variable (struct varobj *var, enum varobj_display_formats format)
 {
-  return cplus_value_of_variable (var);
+  return cplus_value_of_variable (var, format);
 }
 

 extern void _initialize_varobj (void);
Index: gdb/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.996
diff -u -r1.996 Makefile.in
--- gdb/Makefile.in     26 Mar 2008 14:53:28 -0000      1.996
+++ gdb/Makefile.in     2 Apr 2008 18:42:28 -0000
@@ -3219,7 +3219,7 @@
        $(mi_getopt_h) $(remote_h)
        $(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/mi/mi-cmd-target.c
 mi-cmd-var.o: $(srcdir)/mi/mi-cmd-var.c $(defs_h) $(mi_cmds_h) $(ui_out_h) \
-       $(mi_out_h) $(varobj_h) $(value_h) $(gdb_string_h)
+       $(mi_out_h) $(varobj_h) $(value_h) $(gdb_string_h) $(mi_getopt_h)
        $(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/mi/mi-cmd-var.c
 mi-console.o: $(srcdir)/mi/mi-console.c $(defs_h) $(mi_console_h) \
        $(gdb_string_h)
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.478
diff -u -r1.478 gdb.texinfo
--- gdb/doc/gdb.texinfo 26 Mar 2008 18:37:47 -0000      1.478
+++ gdb/doc/gdb.texinfo 2 Apr 2008 18:42:31 -0000
@@ -19996,6 +19996,7 @@
 Sets the output format for the value of the object @var{name} to be
 @var{format-spec}.
 
+@anchor{-var-set-format}
 The syntax for the @var{format-spec} is as follows:
 
 @smallexample
@@ -20172,12 +20173,16 @@
 @subsubheading Synopsis
 
 @smallexample
- -var-evaluate-expression @var{name}
+ -var-evaluate-expression [-f @var{format-spec}] @var{name}
 @end smallexample
 
 Evaluates the expression that is represented by the specified variable
-object and returns its value as a string.  The format of the
-string can be changed using the @code{-var-set-format} command.
+object and returns its value as a string.  The format of the string
+can be specified with the @samp{-f} option.  The possible values of 
+this option are the same as for @code{-var-set-format} 
+(@pxref{-var-set-format}).  If the @samp{-f} option is not specified,
+the current display format will be used.  The current display format 
+can be changed using the @code{-var-set-format} command.
 
 @smallexample
  value=@var{value}
@@ -20230,7 +20235,7 @@
 object names, all existing variable objects are updated, except
 for frozen ones (@pxref{-var-set-frozen}).  The option
 @var{print-values} determines whether both names and values, or just
-names are printed.  The possible values of this options are the same
+names are printed.  The possible values of this option are the same
 as for @code{-var-list-children} (@pxref{-var-list-children}).  It is
 recommended to use the @samp{--all-values} option, to reduce the
 number of MI commands needed on each program stop.
Index: gdb/mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.47
diff -u -r1.47 mi-cmd-var.c
--- gdb/mi/mi-cmd-var.c 26 Mar 2008 14:51:28 -0000      1.47
+++ gdb/mi/mi-cmd-var.c 2 Apr 2008 18:42:31 -0000
@@ -28,6 +28,7 @@
 #include "value.h"
 #include <ctype.h>
 #include "gdb_string.h"
+#include "mi-getopt.h"
 
 const char mi_no_values[] = "--no-values";
 const char mi_simple_values[] = "--simple-values";
@@ -195,13 +196,37 @@
   return MI_CMD_DONE;
 }
 
+/* Parse a string argument into a format value.  */
+
+static enum varobj_display_formats
+mi_parse_format (const char *arg)
+{
+  if (arg != NULL)
+    {
+      int len;
+
+      len = strlen (arg);
+
+      if (strncmp (arg, "natural", len) == 0)
+       return FORMAT_NATURAL;
+      else if (strncmp (arg, "binary", len) == 0)
+       return FORMAT_BINARY;
+      else if (strncmp (arg, "decimal", len) == 0)
+       return FORMAT_DECIMAL;
+      else if (strncmp (arg, "hexadecimal", len) == 0)
+       return FORMAT_HEXADECIMAL;
+      else if (strncmp (arg, "octal", len) == 0)
+       return FORMAT_OCTAL;
+    }
+
+  error (_("Must specify the format as: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
+}
+
 enum mi_cmd_result
 mi_cmd_var_set_format (char *command, char **argv, int argc)
 {
   enum varobj_display_formats format;
-  int len;
   struct varobj *var;
-  char *formspec;
 
   if (argc != 2)
     error (_("mi_cmd_var_set_format: Usage: NAME FORMAT."));
@@ -212,25 +237,8 @@
   if (var == NULL)
     error (_("mi_cmd_var_set_format: Variable object not found"));
 
-  formspec = argv[1];
-  if (formspec == NULL)
-    error (_("mi_cmd_var_set_format: Must specify the format as: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
-
-  len = strlen (formspec);
-
-  if (strncmp (formspec, "natural", len) == 0)
-    format = FORMAT_NATURAL;
-  else if (strncmp (formspec, "binary", len) == 0)
-    format = FORMAT_BINARY;
-  else if (strncmp (formspec, "decimal", len) == 0)
-    format = FORMAT_DECIMAL;
-  else if (strncmp (formspec, "hexadecimal", len) == 0)
-    format = FORMAT_HEXADECIMAL;
-  else if (strncmp (formspec, "octal", len) == 0)
-    format = FORMAT_OCTAL;
-  else
-    error (_("mi_cmd_var_set_format: Unknown display format: must be: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
-
+  format = mi_parse_format (argv[1]);
+  
   /* Set the format of VAR to given format */
   varobj_set_display_format (var, format);
 
@@ -493,15 +501,58 @@
 {
   struct varobj *var;
 
-  if (argc != 1)
-    error (_("mi_cmd_var_evaluate_expression: Usage: NAME."));
+  enum varobj_display_formats format;
+  int formatFound;
+  int optind;
+  char *optarg;
+    
+  enum opt
+    {
+      OP_FORMAT
+    };
+  static struct mi_opt opts[] =
+  {
+    {"f", OP_FORMAT, 1},
+    { 0, 0, 0 }
+  };
+
+  /* Parse arguments */
+  format = FORMAT_NATURAL;
+  formatFound = 0;
+  optind = 0;
+  while (1)
+    {
+      int opt = mi_getopt ("-var-evaluate-expression", argc, argv, opts, &optind, &optarg);
+      if (opt < 0)
+       break;
+      switch ((enum opt) opt)
+      {
+       case OP_FORMAT:
+         if (formatFound)
+           error (_("Cannot specify format more than once"));
+   
+         format = mi_parse_format (optarg);
+         formatFound = 1;
+         break;
+      }
+    }
 
-  /* Get varobj handle, if a valid var obj name was specified */
-  var = varobj_get_handle (argv[0]);
+  if (optind >= argc)
+    error (_("Usage: [-f FORMAT] NAME"));
+   
+  if (optind < argc - 1)
+    error (_("Garbage at end of command"));
+ 
+     /* Get varobj handle, if a valid var obj name was specified */
+  var = varobj_get_handle (argv[optind]);
   if (var == NULL)
-    error (_("mi_cmd_var_evaluate_expression: Variable object not found"));
+    error (_("Variable object not found"));
+   
+  if (formatFound)
+    ui_out_field_string (uiout, "value", varobj_get_formatted_value (var, format));
+  else
+    ui_out_field_string (uiout, "value", varobj_get_value (var));
 
-  ui_out_field_string (uiout, "value", varobj_get_value (var));
   return MI_CMD_DONE;
 }