Re: Variable objects laziness

[Daniel Jacobowitz <drow@false.org>]

On Tue, Nov 14, 2006 at 04:43:24PM +0300, Vladimir Prus wrote:
> At the moment, MI's variable objects are too eager. For example, if you create 
> a variable object corresponding to a structure, gdb will read the entire 
> structure from the memory, even though it never does anything with that value 
> directly. Structure values are not compared, and you can't access them other 
> than by creating children variable objects.
> 
> For example, if you have a huge structure containing two structure fields, gdb 
> will read all the outer structure even though you never open one of the 
> children structures.
> 
> Worse, the code for fetching the values is scattered over a number of places.

I think this is a good patch.  I'm going to wait a few days before
approving this, however, in case any of the other MI users on the list
have comments.

Neither before nor after this patch do I really understand what
"type_changeable" means so I'll trust you've got it right :-)

How does reading the children instead of the parent affect bitfields?
Will we do eight reads from the target for this?

  struct x {
    unsigned char a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1;
  };

How about unions?

> +static int
> +install_new_value (struct varobj *var, struct value *value, int initial);

No newline, since this is a prototype.

> +	value = evaluate_type (var->root->exp);
> +
> +      var->type = value_type (value);

Space/tabs mismatch.

> -      var->type = value_type (var->value);
> +      install_new_value (var, value, 1 /* Initial assignment. */);

Probably don't need the period here, since this isn't supposed to be a
full sentence.  If you did need the period, you'd need another space
after it.

> +      /* All types are the editable must also be changeable.  */

"All types that are editable"

> +      /* Value of a changeable variable object must not be lazy.  */

"The value of"

> +      /* The new value may be lazy, gdb_value_assign, or 
> +	 rather value_contents, will take care of this.  */

Don't use a comma to separate what are basically independent sentences;
you should use a period between "lazy" and "gdb_value_assign".

> +/** Assign new value to a variable object.  If INITIAL is non-zero,

Javadoc?  Not here :-)  Article missing, "Assign a new value" or
"Assign VALUE to VAR".

> +    this is first assignement after the variable object was just

"is the first".

> +  /* The new value might be lazy.  If the type is changeable,
> +     that is we'll be comparing values of this type, fetch the
> +     value now.  Otherwise, on the next update the old value
> +     will be lazy, which means we've lost that old value.
> +     We do this for frozen varobjs as well, since this
> +     function is only called when it's decided that we need
> +     to fetch the new value of a frozen variable.  */

Stray frozen varobjs comment.

> +	  /* Set the value to NULL, so that for the next -var-update,
> +	     we don't try to compare the new value with this value,
> +	     that we can't even read.  */

"this value that we couldn't".

> +  /* If the type is changeable, compare the old and the new values.
> +     If the type of varobj changed, no point in comparing values.  */

"the type of the varobj".

> +  if (!initial && changeable)
> +    {
> +      /* If the value of the varobj was set by -var-set-value, then the 
> +	 value in the varobj and in the target is the same.  However, that value
> +	 is different from the value that the varobj had after the previous
> +	 -var-update. So need to the varobj as changed.  */	 
> +      if (var->updated)
> +	changed = 1;

"need to mark the".  Why is this true?  I would have thought we only
needed to mark the varobj as changed if the value assigned to it was
different from the previous one.  Oh, I see this was what it did
before.  OK.

> +  /* We must always keep new values, since children depend on it. */

"values" is plural, "it" is singular, so this sounds strange; probably
"must always keep the new value".

-- 
Daniel Jacobowitz
CodeSourcery