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

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

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