This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Cleanup varobj children handling
- From: Daniel Jacobowitz <drow at false dot org>
- To: Vladimir Prus <ghost at cs dot msu dot su>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Tue, 26 Dec 2006 11:10:41 -0500
- Subject: Re: Cleanup varobj children handling
- References: <200612082300.06688.ghost@cs.msu.su> <eme90g$ras$1@sea.gmane.org>
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