Re: Variable objects laziness

Daniel Jacobowitz
2006-11-14T20:47:32Z

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