Cleanup varobj children handling

[Vladimir Prus]

On Tuesday 26 December 2006 19:10, Daniel Jacobowitz wrote:
> 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.

Fixed.

> > -  /* 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.

Unfortunately, my emacs immediately shows

	VEC (varobj_p)

in red, because I have a some code to catch the (numerous) cases
when I try to use C++ coding style. I guess it will be a bit hard to
to suppress this check for VEC.


> > +  /* 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.

Yes, I need new elements to be NULL.

> > @@ -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.

I've changed to use VEC_replace. I don't see much improvement, though.

> > +  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.

I personally find VEC_iterate to be less clear -- because it does not
correspond to any iteration pattern in any language I know. Do you insist
on using it?

> > -  /* 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?

Here's what happens. The old code used vpush to construct the result,
and then a loop ("revert the order" comment above) that vpops elements
and places them to the right position. The new code uses VEC_safe_push.
We don't need to reverse the order -- because with VEC we have
random access anyway. So, no order change here.

However, the order of children is different. Old code used a hand-made list,
with each new constructed child added the the front of the list. New code
uses VEC, with each new child added at the end of the list.

So, for children we iterate them in natural order and push them to working
stack. We pop the last child, see that it changed, and push it to the result vector.
At this point, the order of children of any given varobj is reversed.

I've just worked this around by pushing the children in reverse order.

> > @@ -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?

Ick! You're right, this will segfault. Not a common case though -- I don't think
anybody's going to delete select children of a varobj. Fixed.

Please find the revised patch attached, as well as delta relative to 
the previous version.

- Volodya


-------------- next part --------------
A non-text attachment was scrubbed...
Name: path_2_children__gdb_mainline.diff
Type: text/x-diff
Size: 12938 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20061226/1658b8ed/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: path_2_children_delta.diff
Type: text/x-diff
Size: 6876 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20061226/1658b8ed/attachment-0001.bin>