Cleanup varobj children handling

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Cleanup varobj children handling

Vladimir Prus

This patch changes varobj.c to use VEC for hodling children, thereby making three
functions unnecessary. No regressions in MI tests. OK?

- Volodya

        gdb/
        * varobj.c: Include "vec.h".
        (varobj_p): New typedef, declare vector of those.
        (struct varobj): Use vector for the 'children' member.
        (child_exists): Remove.
        (save_child_in_parent): Remove.
        (remove_child_from_parent): Remove.
        (varobj_list_children): Adjust to work work vector.
        (varobj_update): Likewise.
        (delete_variable_1): Likewise.
        * Makefile.in (varobj.o): Update dependencies.

        testsuite/
        * gdb.mi/mi-var-child.exp: Adjust to the change in order
        of varobjs reported by -var-update.
        * gdb.mi/mi2-var-child.exp: Likewise.

path_2_children__gdb_mainline.diff (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Vladimir Prus
Vladimir Prus wrote:

>
> This patch changes varobj.c to use VEC for hodling children, thereby
> making three functions unnecessary. No regressions in MI tests. OK?

I forgot to remove some more dead code -- struct varobj_child is no longer
needed. This patch removes it too.

- Volodya

        gdb/
        * varobj.c: Include "vec.h".
        (varobj_p): New typedef, declare vector of those.
        (struct varobj): Use vector for the 'children' member.
        (child_exists): Remove.
        (save_child_in_parent): Remove.
        (remove_child_from_parent): Remove.
        (struct varobj_child): Remove.
        (varobj_list_children): Adjust to work work vector.
        (varobj_update): Likewise.
        (delete_variable_1): Likewise.
        * Makefile.in (varobj.o): Update dependencies.

        testsuite/
        * gdb.mi/mi-var-child.exp: Adjust to the change in order
        of varobjs reported by -var-update.
        * gdb.mi/mi2-var-child.exp: Likewise.

path_2_children__gdb_mainline.diff (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Vladimir Prus
Vladimir Prus wrote:

> Vladimir Prus wrote:
>
>>
>> This patch changes varobj.c to use VEC for hodling children, thereby
>> making three functions unnecessary. No regressions in MI tests. OK?
>
> I forgot to remove some more dead code -- struct varobj_child is no longer
> needed. This patch removes it too.
>
> - Volodya
>
>         gdb/
>         * varobj.c: Include "vec.h".
>         (varobj_p): New typedef, declare vector of those.
>         (struct varobj): Use vector for the 'children' member.
>         (child_exists): Remove.
>         (save_child_in_parent): Remove.
>         (remove_child_from_parent): Remove.
>         (struct varobj_child): Remove.
>         (varobj_list_children): Adjust to work work vector.
>         (varobj_update): Likewise.
>         (delete_variable_1): Likewise.
>         * Makefile.in (varobj.o): Update dependencies.
>
>         testsuite/
>         * gdb.mi/mi-var-child.exp: Adjust to the change in order
>         of varobjs reported by -var-update.
>         * gdb.mi/mi2-var-child.exp: Likewise.

PING? The above was posted 10 days ago.

- Volodya




Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Vladimir Prus
In reply to this post by Vladimir Prus
Vladimir Prus wrote:

>
> This patch changes varobj.c to use VEC for hodling children, thereby
> making three functions unnecessary. No regressions in MI tests. OK?

Here's a revised patch, that additionally replaces 'vstack' code in varobj.c
with VEC(). All in all, some 130 lines of code gets wiped now. OK?

- Volodya

        gdb/
        * varobj.c: Include "vec.h".
        (varobj_p): New typedef, declare vector of those.
        (struct varobj): Use vector for the 'children' member.
        (child_exists): Remove.
        (save_child_in_parent): Remove.
        (remove_child_from_parent): Remove.
        (struct varobj_child): Remove.
        (struct vstack): Remove.
        (vpush, vpop): Remove.
        (varobj_list_children): Adjust to work work vector.
        (varobj_update): Likewise. Use vectors for
        working stack and result.
        (delete_variable_1): Likewise.
        * Makefile.in (varobj.o): Update dependencies.

        testsuite/
        * gdb.mi/mi-var-child.exp: Adjust to the change in order
        of varobjs reported by -var-update.
        * gdb.mi/mi2-var-child.exp: Likewise.

path_2_children__gdb_mainline.diff (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Nick Roberts
 > > This patch changes varobj.c to use VEC for hodling children, thereby
 > > making three functions unnecessary. No regressions in MI tests. OK?
 >
 > Here's a revised patch, that additionally replaces 'vstack' code in varobj.c
 > with VEC(). All in all, some 130 lines of code gets wiped now. OK?

I'm not familiar with vec.c yet - perhaps you and Daniel J discussed this issue
privately - all I can say is that varobj. c has the comment:

/* Every variable keeps a linked list of its children, described
   by the following structure. */
/* FIXME: Deprecated.  All should use vlist instead */

Buy perhaps this is not relevant.

--
Nick                                           http://www.inet.net.nz/~nickrob
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Vladimir Prus
On Friday 22 December 2006 01:37, Nick Roberts wrote:

>  > > This patch changes varobj.c to use VEC for hodling children, thereby
>  > > making three functions unnecessary. No regressions in MI tests. OK?
>  >
>  > Here's a revised patch, that additionally replaces 'vstack' code in varobj.c
>  > with VEC(). All in all, some 130 lines of code gets wiped now. OK?
>
> I'm not familiar with vec.c yet - perhaps you and Daniel J discussed this issue
> privately - all I can say is that varobj. c has the comment:
>
> /* Every variable keeps a linked list of its children, described
>    by the following structure. */
> /* FIXME: Deprecated.  All should use vlist instead */
>
> Buy perhaps this is not relevant.

The comment was written before vec.c was available, so it naturally
does not suggest vec.c.

- Volodya
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Nick Roberts
 > > /* Every variable keeps a linked list of its children, described
 > >    by the following structure. */
 > > /* FIXME: Deprecated.  All should use vlist instead */
 > >
 > > Buy perhaps this is not relevant.
 >
 > The comment was written before vec.c was available, so it naturally
 > does not suggest vec.c.

Before your change varobj.c had two (equivalent?) types of data structure:
vstack and vlist.  After your change varobj.c has two (equivalent?) types of
data structure: VEC and vlist.

Should VEC not also be used for vlist?

--
Nick                                           http://www.inet.net.nz/~nickrob
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Vladimir Prus
On Friday 22 December 2006 09:44, Nick Roberts wrote:

>  > > /* Every variable keeps a linked list of its children, described
>  > >    by the following structure. */
>  > > /* FIXME: Deprecated.  All should use vlist instead */
>  > >
>  > > Buy perhaps this is not relevant.
>  >
>  > The comment was written before vec.c was available, so it naturally
>  > does not suggest vec.c.
>
> Before your change varobj.c had two (equivalent?) types of data structure:
> vstack and vlist.  

Actually, three: vstack, varobj_child and vlist.

> After your change varobj.c has two (equivalent?) types of
> data structure: VEC and vlist.

That's one data type off, plus vec.h is generic code, not private to varobj.c

> Should VEC not also be used for vlist?

Ultimately, yes. But I primarily wanted to cleanup the list of children, so open
path for other changes.

vlist does not get in my way, yet ;-)

- Volodya
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Daniel Jacobowitz-2
In reply to this post by Vladimir Prus
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
Reply | Threaded
Open this post in threaded view
|

Re: 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



path_2_children__gdb_mainline.diff (13K) Download Attachment
path_2_children_delta.diff (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup varobj children handling

Daniel Jacobowitz-2
On Tue, Dec 26, 2006 at 11:49:14PM +0300, Vladimir Prus wrote:
> 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?

No, this is OK to leave alone.  If you think of the integer index as
opaque, it's really not that unlike a C++ iterator; since we can't make
*ix work, we use two variables.


> +  /* If we're called when the list of children is not yet initialized,
> +     allocated enough elements in it.  */

Allocate, not allocated - this is what we're doing, not what we've
done.

> +      /* Push any children.  Use reverse order so that first
> + child is popped from the work stack first, and so
> + will be added to result first.  This does not
> + affect correctness, just "nicer".  */

"so that the first child"

> +  /* Child may be NULL is explicitly deleted by -var-delete.  */

"if", not is.

> +      /* Update this variable, unless it's root, which is already
> + updated.  */

"unless it's a root"

Otherwise OK, with a changelog entry.

--
Daniel Jacobowitz
CodeSourcery