Re: Variable objects laziness

[Nick Roberts <nickrob@snap.net.nz>]

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