From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28509 invoked by alias); 5 Jan 2007 01:09:46 -0000 Received: (qmail 28498 invoked by uid 22791); 5 Jan 2007 01:09:45 -0000 X-Spam-Check-By: sourceware.org Received: from viper.snap.net.nz (HELO viper.snap.net.nz) (202.37.101.8) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 05 Jan 2007 01:09:38 +0000 Received: from kahikatea.snap.net.nz (p202-124-120-54.snap.net.nz [202.124.120.54]) by viper.snap.net.nz (Postfix) with ESMTP id 2EDDD3D844C; Fri, 5 Jan 2007 14:09:34 +1300 (NZDT) Received: by kahikatea.snap.net.nz (Postfix, from userid 500) id 69D894F6CD; Fri, 5 Jan 2007 14:09:32 +1300 (NZDT) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Message-ID: <17821.42314.506114.619107@kahikatea.snap.net.nz> Date: Fri, 05 Jan 2007 01:09:00 -0000 To: Vladimir Prus Cc: Daniel Jacobowitz , gdb-patches@sources.redhat.com Subject: Re: RFC: MI - Detecting change of string contents with variable objects In-Reply-To: <200701050004.30631.ghost@cs.msu.su> References: <17797.65268.689590.797944@kahikatea.snap.net.nz> <20070104042038.GA3918@nevyn.them.org> <17820.39505.966623.305338@kahikatea.snap.net.nz> <200701050004.30631.ghost@cs.msu.su> X-Mailer: VM 7.19 under Emacs 22.0.92.5 X-IsSubscribed: yes 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: 2007-01/txt/msg00164.txt.bz2 > > + =A0 /* Last print value */ >=20 > Dot and two spaces ;-) OK=20 >... > > + static char *value_get_print_value (struct value *value, > > + =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0enum varobj_display_formats format); > > +=20 >=20 > I don't see a comment to this function either here or at the definition > point. Too many comments get in the way of the code and I think the name is fairly self explanatory. Most of the simple functions don't have a comment and AF= AIK a comment for them is not a GNU or GDB coding standard. > > =A0 static int type_changeable (struct varobj *var); > > =A0=20 > > =A0 /* C implementation */ > > *************** install_new_value (struct varobj *var, s > > *** 978,1003 **** > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0changed =3D 1; > > =A0 =A0=A0=A0=A0=A0=A0 =A0else > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0{ > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0gdb_assert (!value_lazy (var->value)= ); > > - =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0gdb_assert (!value_lazy (value)); >=20 > Did you remove this because it failed? If so, I'd like to understand why. > I think this assert is needed -- if the value is lazy, then even if > printing code will fetch the value, you'll be comparing current value wi= th > current value. That's a definite bug, so must be asserted. I removed it accidentally. I've put it back. > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0 > > ! =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0if (!value_contents_equal (var->value,= value)) > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0changed =3D 1; > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0} > > =A0 =A0=A0=A0=A0=A0=A0} > > =A0 =A0 =A0 } > > ! =A0 =A0=20 > > =A0 =A0 /* We must always keep the new value, since children depend on= it. =A0*/ > > =A0 =A0 if (var->value !=3D NULL) > > =A0 =A0 =A0 value_free (var->value); > > =A0 =A0 var->value =3D value; > > =A0 =A0 var->updated =3D 0; > > ! =A0=20 > > =A0 =A0 gdb_assert (!var->value || value_type (var->value)); >=20 > Is that a formatting change above? I've just removed unnecessay spaces. The real change is replacing value_contents_equal (but what's with all the underscores!). > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0if (strcmp (var->print= _value, print_value)) >=20 > Can you use=20 >=20 > strcmp (var->print_value, print_value) !=3D 0 Is that more legible? I sometimes see "if (fi !=3D NULL)" but "if (fi)" seems clearer. Maybe it comes from programming in Lisp for Emacs. > for legibility? >=20 > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0{ > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0 =A0xfree (var->pr= int_value); > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0 =A0var->print_val= ue =3D print_value; > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0 =A0changed =3D 1; > > ! =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0} >=20 > So, if values differ you xfree the old one and assign the new one. If the > values are the same -- where is 'print_value' freed? It's not; it's a legacy of earlier code. I'll change it. --=20 Nick http://www.inet.net.nz/~nick= rob