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

[Keith Seitz <keiths@redhat.com>]

On 02/14/2014 12:44 AM, Yao Qi wrote:
>   V2:
>   - Fix changelog entry.
>   - Add comments.
>   - Use XNEW.
>   - Fix typo.
>   - Update copyright year for new added file.
>
> gdb:
>
> 2014-02-14  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 (varobj_get_iterator): Add one more argument
> 	'lang_ops'.  All callers updated.
> 	Return iterator for available children.

This looks good to me, but I have one little thing I'd like to ask...

> +static int
> +varobj_value_unavailable (struct value *val)
> +{

[snip]

> +/* 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))
> +	{

Maybe it's just me (and maybe because I am especially slow today), but I 
find the use of double-negatives slow me/my comprehension down. The 
above test reads, "if child_value is NULL or child_value is not 
unavailable..."

What do you think about inverting the name of this function and the test 
like so:

   if (child_value == NULL || varobj_value_available (child_value))
      /* ... */

Then that reads, "if child_value is NULL or the value is available," 
which is immensely easier for me to comprehend. [But maybe I'm the only 
one?]

Keith