This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [Patch] Try2: -var-evaluate-expression [-f FORMAT] NAME
- From: Vladimir Prus <ghost at cs dot msu dot su>
- To: "Marc Khouzam" <marc dot khouzam at ericsson dot com>
- Cc: gdb-patches at sources dot redhat dot com, Eli Zaretskii <eliz at gnu dot org>
- Date: Fri, 4 Apr 2008 17:43:36 +0400
- Subject: Re: [Patch] Try2: -var-evaluate-expression [-f FORMAT] NAME
- References: <6D19CA8D71C89C43A057926FE0D4ADAA0429101B@ecamlmw720.eamcs.ericsson.se>
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