This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Cleanup varobj children handling


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]