Re: Cleanup varobj children handling

Daniel Jacobowitz
2006-12-26T16:10:52Z

Thanks for doing this, I think it's a good cleanup. To answer Nick's
questions: Vlad and I hadn't talked about this change, but I agree with
him. The FIXME about should use vlists goes along with this one that
Vlad deleted:

/* FIXME: This should be a generic add to list */

vec.h is basically the generic list structure the code was asking for.

On Thu, Dec 21, 2006 at 06:25:04PM +0300, Vladimir Prus wrote:
> #include "varobj.h"
> +#include "vec.h"
> +

Extra blank line.

> - /* A list of this object's children */
> - struct varobj_child *children;
> + /* Children of this object. */
> + VEC (varobj_p) *children;

I've been omitting the space before the parenthesis for VEC, since it's
a macro creating a type and syntactically very different from a
function call. I can't remember where I got that habit though! Either
way is fine.

> if (var->num_children == -1)
> - var->num_children = number_of_children (var);
> + {
> + var->num_children = number_of_children (var);
> + }

Braces :-)

> + /* If we're called when the list of children is not yet initialized,
> + allocated enough elements in it. */
> + while (VEC_length (varobj_p, var->children) < var->num_children)
> + VEC_safe_push (varobj_p, var->children, NULL);

You need the NULLs initialized here, right? Otherwise you could use
grow instead, but that doesn't initialize.

> @@ -721,13 +701,18 @@ varobj_list_children (struct varobj *var
> /* Mark as the end in case we bail out */
> *((*childlist) + i) = NULL;
>
> - /* check if child exists, if not create */
> - name = name_of_child (var, i);
> - child = child_exists (var, name);
> - if (child == NULL)
> - child = create_child (var, i, name);
> + varobj_p *existing = VEC_address (varobj_p, var->children) + i;

Please declare existing at the top of the block, instead of in the
middle; we only require C90 compatibility. Do you really need to use
VEC_address here? You could use VEC_index to read it and VEC_replace
to modify it; I find that easier to read than a pointer into the vec's
storage.

> /* If this is a "use_selected_frame" varobj, and its type has changed,
> them note that it's changed. */
> if (type_changed)
> {
> - vpush (&result, *varp);
> - changed++;
> + VEC_safe_push (varobj_p, result, *varp);
> }

Braces again :-) A few other places too.

> + for (i = 0; i < VEC_length (varobj_p, (*varp)->children); ++i)
> + {
> + VEC_safe_push (varobj_p, stack,
> + VEC_index (varobj_p, (*varp)->children, i));
> }

Here and elsewhere, VEC_iterate avoids having to call VEC_index -
that's what it's for.

> - /* Copy from result stack to list */
> - vleft = changed;
> - *cv = vpop (&result);
> - while ((*cv != NULL) && (vleft > 0))
> - {
> - vleft--;
> - cv++;
> - *cv = vpop (&result);
> - }
> - if (vleft)
> - warning (_("varobj_update: assertion failed - vleft <> 0"));
> + cv = *changelist;
>
> - if (changed > 1)
> + for (i = 0; i < changed; ++i)
> {
> - /* Now we revert the order. */
> - for (i = 0; i < changed; i++)
> - *(*changelist + i) = *(templist + changed - 1 - i);
> - *(*changelist + changed) = NULL;
> + *cv = VEC_index (varobj_p, result, i);
> + gdb_assert (*cv != NULL);
> + ++cv;
> }
> + *cv = 0;
>
> if (type_changed)
> return -2;

It looks to me as if the old code was somewhat careful about the
ordering, and the old ordering was a bit nicer. But it also looks to
me as if you're preserving the same ordering - vpop takes what would be
the "last" item in the vstack. So why does this patch reverse the order
for the testsuite?

> @@ -1227,7 +1179,7 @@ delete_variable_1 (struct cpstack **resu
> discarding the list afterwards */
> if ((remove_from_parent_p) && (var->parent != NULL))
> {
> - remove_child_from_parent (var->parent, var);
> + VEC_replace (varobj_p, var->parent->children, var->index, NULL);
> }
>
> if (var->obj_name != NULL)

I think that this will cause crashes in -var-update, since that walks
the list of children without checking for NULL. If I'm right, you
probably want to add NULL checks rather than use VEC_ordered_remove,
so that the indexes still tell you which child it is?

--
Daniel Jacobowitz
CodeSourcery