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

[Vladimir Prus <ghost@cs.msu.su>]

Marc Khouzam wrote:

> 2008-04-01  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.

Is '*(value_of_variable)' really a name of a function? :-)

>         (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.


> +/* 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?

> +
> +static enum varobj_display_formats
> +mi_parse_format (const char *arg)
> +{
> +  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;
> +  else
> +    error (_("Unknown display format: must be: \"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;
>  
> @@ -216,21 +239,8 @@
>    if (formspec == NULL)
>      error (_("mi_cmd_var_set_format: Must specify the format as: \"natural\", \"binary\",
\"decimal\", \"hexadecimal\", or \"octal\""));

Can we drop this...

>  
> -  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 (formspec);

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

>    /* Set the format of VAR to given format */
>    varobj_set_display_format (var, format);
>  
> @@ -493,15 +503,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 ("mi_cmd_var_evaluate_expression", argc, argv, opts, &optind, &optarg);
> +      if (opt < 0)
> +       break;
> +      switch ((enum opt) opt)
> +      {
> +       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?

> +         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 (_("mi_cmd_var_evaluate_expression: Usage: [-f FORMAT] NAME"));
> +   
> +  if (optind < argc - 1)
> +    error (_("mi_cmd_var_evaluate_expression: 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"));
> +   
> +  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.

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

- Volodya