From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10735 invoked by alias); 15 Nov 2006 02:45:34 -0000 Received: (qmail 10724 invoked by uid 22791); 15 Nov 2006 02:45:33 -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; Wed, 15 Nov 2006 02:45:27 +0000 Received: from kahikatea.snap.net.nz (p202-124-125-100.snap.net.nz [202.124.125.100]) by viper.snap.net.nz (Postfix) with ESMTP id 3ED553D8447; Wed, 15 Nov 2006 15:45:47 +1300 (NZDT) Received: by kahikatea.snap.net.nz (Postfix, from userid 500) id E9CDBBE3F0; Wed, 15 Nov 2006 15:41:27 +1300 (NZDT) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <17754.32341.141757.120973@kahikatea.snap.net.nz> Date: Wed, 15 Nov 2006 02:45:00 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Variable objects laziness X-Mailer: VM 7.19 under Emacs 22.0.90.14 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-11/txt/msg00115.txt.bz2 I'm having trouble to understand this patch. If it does more than one thing perhaps you can break it into two patches. * varobj.c (struct varobj): Clarify comment. (my_value_equal): Remove. (install_new_value): New. New function presumably. (type_of_child): Remove. (varobj_create): Use install_new_value. (varobj_set_value): Use value_contents_equal, not my_value_equal. Previously someone (Mark Kettenis?) has gone to a lot of trouble to replace value_contents_equal with my_value_equal why do you think it's not needed? (varobj_update): Use install_new_value. (create_child): Likewise. Inline type_of_child here. (value_of_child): Don't fetch the value. (c_value_of_root): Likewise. (c_value_of_variable): Likewise. Index: varobj.c =================================================================== RCS file: /cvs/src/src/gdb/varobj.c,v retrieving revision 1.60 diff -u -p -r1.60 varobj.c --- varobj.c 3 May 2006 22:59:38 -0000 1.60 +++ varobj.c 14 Nov 2006 13:38:35 -0000 @@ -101,7 +101,9 @@ struct varobj /* The type of this variable. This may NEVER be NULL. */ struct type *type; - /* The value of this expression or subexpression. This may be NULL. */ + /* The value of this expression or subexpression. This may be NULL. + Invariant: if type_changeable (this) is non-zero, the value is either + NULL, or not lazy. */ I don't understand the replacement comment ... +/** Assign new value to a variable object. If INITIAL is non-zero, + this is first assignement after the variable object was just + created, or changed type. In that case, just assign the value + and return 0. + Otherwise, assign the value and if type_changeable returns non-zero, + find if the new value is different from the current value. + Return 1 if so, and 0 is the values are equal. */ +static int +install_new_value (struct varobj *var, struct value *value, int initial) +{ I don't understand this comment either. INITIAL can be type_changed i.e not really initial. Can you give it more structure and not refer to internals like type_changeable? ... + if (CPLUS_FAKE_CHILD (var)) + changeable = 0; + else + changeable = type_changeable (var); type_changeable returns 0 if CPLUS_FAKE_CHILD (var) is true anyway so do you need this clause? As a whole the patch seems to lack clarity (although that might partly be a reflection on my abilities!) -- Nick http://www.inet.net.nz/~nickrob