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

[Daniel Jacobowitz <drow@false.org>]

On Thu, Jan 24, 2008 at 04:35:37PM -0500, Marc Khouzam wrote:
> New version based on comments.
> This patch also includes tests and documentation update.
> I ran regression and got no failure in the new tests
> that I added.
> 
> Comments?

This looks basically OK to me.  Just minor comments.

> +extern char *varobj_get_formatted_value (struct varobj *var,
> +                                enum varobj_display_formats format);

You've almost got GNU indentation style, but not quite.  We replace
leading eight space sequences with tabs, and parameters should line
up.  Of course that doesn't quite fit here, so some rule has to bend;
it doesn't really matter which, but please use the tabs.

> +struct cleanup_set_format_struct
> +{
> +       struct varobj *var;
> +       enum varobj_display_formats format;
> +};

Two space indent there.

> +  varobj_set_display_format (cleanup_struct->var, 
> +                                        cleanup_struct->format);

These don't line up - I suspect a different tab width somewhere along
the line?

> Index: gdb/doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.459
> diff -u -r1.459 gdb.texinfo
> --- gdb/doc/gdb.texinfo 23 Jan 2008 11:26:29 -0000      1.459
> +++ gdb/doc/gdb.texinfo 24 Jan 2008 21:26:46 -0000
> @@ -19995,12 +19995,16 @@
>  @subsubheading Synopsis
>  
>  @smallexample
> - -var-evaluate-expression @var{name}
> + -var-evaluate-expression @var{name} [-f @var{format-spec}]
>  @end smallexample

The grammar for MI commands says options always come first.  They're
optional, anyway, so how about -var-evaluate-expression [-f
@var{format-spec}] @var{name}?

> Index: gdb/mi/mi-cmd-var.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
> retrieving revision 1.44
> diff -u -r1.44 mi-cmd-var.c
> --- gdb/mi/mi-cmd-var.c 23 Jan 2008 06:13:44 -0000      1.44
> +++ gdb/mi/mi-cmd-var.c 24 Jan 2008 21:26:46 -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";

Yes, we do live in the distant past.  If you add a #include, you have
to manually update gdb/Makefile.in.  Maybe this year we'll get
automatic dependencies.

> +      switch ((enum opt) opt)
> +         {
> +         case OP_FORMAT:
> +               if (formatFound)
> +             error (_("mi_cmd_var_evaluate_expression: cannot specify format more than once"));
> +
> +               format = mi_parse_format (optarg);
> +               formatFound = 1;
> +           break;
> +         }

Funny indentation again.

-- 
Daniel Jacobowitz
CodeSourcery