Re: [PATCH 08/11] Iterator varobj_items by their availability

[Keith Seitz <keiths@redhat.com>]

On 11/23/2013 06:09 PM, Yao Qi wrote:
> This patch adds a new implementation of varobj iterator, which is
> based on the availability of children.
>
> gdb:
>
> 2013-10-24  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
>
> 	* Makefile.in (SFILES): Add varobj-iter-avail.c.
> 	(COMMON_OBS): Add varobj-iter-avail.o.
> 	* varobj-iter-avail.c: New file.
> 	* varobj-iter.h (avail_varobj_get_iterator): Declare.
> 	* varobj.c (dynamic_varobj_has_child_method):

No changes to dynamic_varobj_has_child_method that I can see.
> 	(varobj_get_iterator): Add one more argument 'lang_ops' and
> 	Caller update.
> 	Return iterator for available children.
> ---
>   gdb/Makefile.in         |    4 +-
>   gdb/varobj-iter-avail.c |  160 +++++++++++++++++++++++++++++++++++++++++++++++
>   gdb/varobj-iter.h       |    4 +
>   gdb/varobj.c            |    8 ++-
>   4 files changed, 172 insertions(+), 4 deletions(-)
>   create mode 100644 gdb/varobj-iter-avail.c
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 7d8a365..6d84def 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -772,7 +772,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
>   	ui-out.c utils.c ui-file.h ui-file.c \
>   	user-regs.c \
>   	valarith.c valops.c valprint.c value.c varobj.c common/vec.c \
> -	xml-tdesc.c xml-support.c \
> +	varobj-iter-avail.c xml-tdesc.c xml-support.c \
>   	inferior.c gdb_usleep.c \
>   	record.c record-full.c gcore.c \
>   	jit.c \
> @@ -933,7 +933,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>   	ada-lang.o c-lang.o d-lang.o f-lang.o objc-lang.o \
>   	ada-tasks.o ada-varobj.o c-varobj.o \
>   	ui-out.o cli-out.o \
> -	varobj.o vec.o \
> +	varobj.o varobj-iter-avail.o vec.o \
>   	go-lang.o go-valprint.o go-typeprint.o \
>   	jv-lang.o jv-valprint.o jv-typeprint.o jv-varobj.o \
>   	m2-lang.o opencl-lang.o p-lang.o p-typeprint.o p-valprint.o \
> diff --git a/gdb/varobj-iter-avail.c b/gdb/varobj-iter-avail.c
> new file mode 100644
> index 0000000..0cd2e23
> --- /dev/null
> +++ b/gdb/varobj-iter-avail.c
> @@ -0,0 +1,160 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "gdb_assert.h"
> +#include "value.h"
> +#include "valprint.h"
> +#include "varobj.h"
> +#include "varobj-iter.h"
> +
> +/* A dynamic varobj iterator "class" for available-children-only
> +   varobjs.  This inherits struct varobj_iter.  */
> +
> +struct avail_varobj_iter
> +{
> +  /* The 'base class'.  */
> +  struct varobj_iter base;
> +
> +  const struct lang_varobj_ops *lang_ops;

This field should also have a (trivial) comment.

> +};
> +
> +/* Returns true if VAL is not interesting for an
> +   available-children-only varobj.  */
> +
> +static int
> +varobj_value_unavailable (struct value *val)
> +{
> +  volatile struct gdb_exception e;
> +  int unavail = 0;
> +
> +  gdb_assert (val != NULL);
> +
> +  /* value_entirely_unavailable may need to try reading the value,
> +     which may throw for example, a memory error, e.g., when not
> +     inspecting a traceframe, we try to read the pointee of a dangling
> +     or NULL pointer.  */
> +  TRY_CATCH (e, RETURN_MASK_ERROR)
> +    {
> +      struct type *type = value_type (val);
> +
> +      unavail = (value_entirely_unavailable (val)
> +		 /* A scalar object that does not have all bits available
> +		    is also considered unavailable, because all bits
> +		    contribute to its representation.  */
> +		 || (val_print_scalar_type_p (type)
> +		     && !value_bytes_available (val,
> +						value_embedded_offset (val),
> +						TYPE_LENGTH (type))));
> +    }
> +  if (e.reason < 0)
> +    return 1;
> +  else
> +    return unavail;
> +}
> +
> +/* Implementation of the `next' method for available-children-only
> +   varobj iterators.  */
> +
> +static varobj_item *
> +avail_varobj_iter_next (struct varobj_iter *self)
> +{
> +  struct avail_varobj_iter *dis = (struct avail_varobj_iter *) self;
> +  int num_children = dis->lang_ops->number_of_children (self->var);
> +  int raw_index = self->next_raw_index;
> +  varobj_item *item = NULL;
> +
> +  for (; raw_index < num_children; raw_index++)
> +    {
> +      struct value *child_value
> +	= dis->lang_ops->value_of_child (self->var, raw_index);
> +
> +      /* The "fake" children will have NULL values.  */
> +      if (child_value == NULL || !varobj_value_unavailable (child_value))
> +	{
> +	  item = xmalloc (sizeof *item);
> +
> +	  item->name
> +	    = dis->lang_ops->name_of_child (self->var, raw_index);
> +	  item->value = child_value;
> +
> +	  raw_index++;
> +	  self->next_raw_index = raw_index;
> +	  break;
> +	}
> +    }
> +  return item;
> +}
> +
> +/* Implementation of the `dtor' method of available-children-only
> +   varobj iterators.  */
> +
> +static void
> +avail_varobj_iter_dtor (struct varobj_iter *self)
> +{
> +  /* nothing to do */
> +}
> +
> +/* The 'vtable' of available-children-only varobj iterators.  */
> +
> +static const struct varobj_iter_ops avail_varobj_iter_ops =
> +{
> +  avail_varobj_iter_dtor,
> +  avail_varobj_iter_next
> +};
> +
> +/* Constructor of available-children-only varobj iterators.  VAR is
> +   the varobj whose children the iterator will be iterating over.  */
> +
> +static void
> +avail_varobj_iter_ctor (struct avail_varobj_iter *self, struct varobj *var,
> +			const struct lang_varobj_ops *lang_ops)
> +{
> +  self->base.var = var;
> +  self->base.ops = &avail_varobj_iter_ops;
> +  self->base.next_raw_index = 0;
> +  self->lang_ops = lang_ops;
> +}

Missing a newline here.

> +/* Allocate and construct an available-children-only varobj iterator.
> +   VAR is the varobj whose children the iterator will be iterating
> +   over.  */
> +
> +static struct avail_varobj_iter *
> +avail_varobj_iter_new (struct varobj *var, const struct lang_varobj_ops *lang_ops)
> +{
> +  struct avail_varobj_iter *self;
> +
> +  self = xmalloc (sizeof (struct avail_varobj_iter));

Please use XNEW.

> +  avail_varobj_iter_ctor (self, var, lang_ops);
> +  return self;
> +}
> +
> +/* Return a new available-children-only varobj iterator suitable to
> +   iterate over VAR's children.  */
> +
> +struct varobj_iter *
> +avail_varobj_get_iterator (struct varobj *var,
> +			   const struct lang_varobj_ops *lang_ops)
> +{
> +  struct avail_varobj_iter *iter;
> +
> +  /* Avoid the needless allocation/dealocation of the iterator if
                                       ^^^^^^^^^^^

typo

> +     there are no raw children to iterator over anyway.  */
                                      ^^^^^^^^
"iterate"

> +  if (lang_ops->number_of_children (var) == 0)
> +    return NULL;
> +
> +  iter = avail_varobj_iter_new (var, lang_ops);
> +  return &iter->base;
> +}
> diff --git a/gdb/varobj-iter.h b/gdb/varobj-iter.h
> index b4bda82..c466225 100644
> --- a/gdb/varobj-iter.h
> +++ b/gdb/varobj-iter.h
> @@ -68,3 +68,7 @@ struct varobj_iter_ops
>   	  xfree (ITER);		       \
>   	}				       \
>       } while (0)
> +
> +struct varobj_iter *
> +  avail_varobj_get_iterator (struct varobj *var,
> +			     const struct lang_varobj_ops *lang_ops);

Needs a comment.

> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 1fd9fd2..ba93eb5 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -768,8 +768,11 @@ dynamic_varobj_has_child_method (struct varobj *var)
>      suitable for iterating over VAR's children.  */
>
>   static struct varobj_iter *
> -varobj_get_iterator (struct varobj *var)
> +varobj_get_iterator (struct varobj *var, const struct lang_varobj_ops *lang_ops)
>   {
> +  if (var->dynamic->available_children_only)
> +    return avail_varobj_get_iterator (var, lang_ops);
> +
>   #if HAVE_PYTHON
>     if (var->dynamic->pretty_printer)
>       return py_varobj_get_iterator (var, var->dynamic->pretty_printer);
> @@ -810,7 +813,8 @@ update_dynamic_varobj_children (struct varobj *var,
>     if (update_children || var->dynamic->child_iter == NULL)
>       {
>         varobj_iter_delete (var->dynamic->child_iter);
> -      var->dynamic->child_iter = varobj_get_iterator (var);
> +      var->dynamic->child_iter = varobj_get_iterator (var,
> +						      var->root->lang_ops);
>
>         varobj_clear_saved_item (var->dynamic);
>
>