From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10690 invoked by alias); 4 Apr 2008 13:44:27 -0000 Received: (qmail 10680 invoked by uid 22791); 4 Apr 2008 13:44:25 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 04 Apr 2008 13:44:02 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.63) (envelope-from ) id 1JhmCt-0001Lp-TG for gdb-patches@sources.redhat.com; Fri, 04 Apr 2008 17:43:55 +0400 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1JhmCi-0001Lc-Os; Fri, 04 Apr 2008 17:43:41 +0400 From: Vladimir Prus To: "Marc Khouzam" Subject: Re: [Patch] Try2: -var-evaluate-expression [-f FORMAT] NAME Date: Fri, 04 Apr 2008 15:17:00 -0000 User-Agent: KMail/1.9.6 (enterprise 0.20070907.709405) Cc: gdb-patches@sources.redhat.com, Eli Zaretskii References: <6D19CA8D71C89C43A057926FE0D4ADAA0429101B@ecamlmw720.eamcs.ericsson.se> In-Reply-To: <6D19CA8D71C89C43A057926FE0D4ADAA0429101B@ecamlmw720.eamcs.ericsson.se> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200804041743.37053.ghost@cs.msu.su> 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/msg00099.txt.bz2 On Wednesday 02 April 2008 23:10:00 Marc Khouzam wrote: >=20 > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 * varobj.h (varobj_get_formatted_value): De= clare. > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 * varobj.c (my_value_of_variable): Added = format parameter. > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 (cplus_value_of_variable): Likewise. > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 (java_value_of_variable): Likewise. > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 (*(value_of_variable)): Likewise. > >=20 > > Is '*(value_of_variable)' really a name of a function? :-) >=20 > After Daniel's comment, I'm not too sure what you guys do=20 > for function pointers that are members....=20 > so I removed this Changelog entry. :-) >=20 > > > +/* Parse a string argument into a format value. =C2=A0*/ > >=20 > > As meta-remark, can you pass the "-p" option to diff when generating pa= tches, > > so that function names are present right in the patch? >=20 >=20 > 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 >=20 > > ..and make mi_parse_format emit an error message both if the passed for= mat > > is NULL and when it's not NULL, but unknown? >=20 > Yes, that is nicer. Done. >=20 >=20 > > > + =C2=A0 =C2=A0 =C2=A0 case OP_FORMAT: > > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (formatFound) > > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error (_("mi_cmd_var_evaluate_ex= pression: cannot specify format more than once")); > > > + =C2=A0=20 > >=20 > > I think the current position is that error messages need not name the n= ame 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? >=20 > Done, but only in this method. Other methods should be cleaned up in a s= eparate patch, right? Right. It's nice to clean up the code you're directly touching, but only th= at, not all existing issues :-) >=20 > > > + =C2=A0if (formatFound) > > > + =C2=A0 =C2=A0ui_out_field_string (uiout, "value", varobj_get_format= ted_value (var, format)); > >=20 > > 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* poi= nter. I think > > we have to free it here. >=20 > 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 als= o a leak in when > calling and not freeing: > varobj_get_expression > varobj_get_type > ... >=20 > How do you suggest we handle this? I can make a patch that attempts to f= ix 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 bec= omes any worse with your patch. > > > + =C2=A0else > > > + =C2=A0 =C2=A0ui_out_field_string (uiout, "value", varobj_get_value = (var)); > > > =C2=A0 > > > - =C2=A0ui_out_field_string (uiout, "value", varobj_get_value (var)); > > > =C2=A0 =C2=A0return MI_CMD_DONE; > >=20 > > The code patch is OK with those changes, thanks! > > Eli, does the doc patch look good for you? >=20 > 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