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

Vladimir Prus
2008-04-04T13:44:27Z

On Wednesday 02 April 2008 23:10:00 Marc Khouzam wrote:
>
> >         * 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?

Right. It's nice to clean up the code you're directly touching, but only that, not all existing
issues :-)

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

Of course, if you prepare such a patch, it would be great, but you are not in any way
obliged to -- this is not a problem that you introduced and it does not becomes any
worse with your patch.

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

The code part of this is OK, thanks. Eli, what about the docs?

- Volodya