From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2953 invoked by alias); 2 Apr 2008 13:23:05 -0000 Received: (qmail 2942 invoked by uid 22791); 2 Apr 2008 13:23:02 -0000 X-Spam-Check-By: sourceware.org Received: from main.gmane.org (HELO ciao.gmane.org) (80.91.229.2) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 02 Apr 2008 13:22:25 +0000 Received: from list by ciao.gmane.org with local (Exim 4.43) id 1Jh2uz-0002tN-Qj for gdb-patches@sources.redhat.com; Wed, 02 Apr 2008 13:22:21 +0000 Received: from 78.158.192.230 ([78.158.192.230]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 02 Apr 2008 13:22:21 +0000 Received: from ghost by 78.158.192.230 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 02 Apr 2008 13:22:21 +0000 To: gdb-patches@sources.redhat.com From: Vladimir Prus Subject: Re: [Patch] Try2: -var-evaluate-expression [-f FORMAT] NAME Date: Wed, 02 Apr 2008 16:26:00 -0000 Message-ID: References: <6D19CA8D71C89C43A057926FE0D4ADAA04290FFC@ecamlmw720.eamcs.ericsson.se> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8Bit User-Agent: KNode/0.10.5 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-04/txt/msg00039.txt.bz2 Marc Khouzam wrote: > 2008-04-01  Marc Khouzam   > >         * 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