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] Try2: -var-evaluate-expression [-f FORMAT] NAME


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



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