Re: [PATCH] gdb: user variables with components of dynamic type

[Joel Brobecker <brobecker@adacore.com>]

Hi Andrew,

> -  /* If type has a dynamic resolved location property
> -     update it's value address.  */
> +  /* If either the WHOLE value, or the COMPONENT value has a dynamic
> +     resolved location property then update the address of the COMPONENT.
> +
> +     If the COMPONENT itself has a dynamic location, and was an
> +     lval_internalvar_component, then we change this to lval_memory.
> +     Usually a component of an internalvar is created non-lazy, and has its
> +     content immediately copied from the parent internalvar.  However,
> +     for components with a dynamic location, the content of the component
> +     is not contained within the parent, but is instead accessed
> +     indirectly.  Further, the component will be created as a lazy value.
> +
> +     By changing the type of the component to lval_memory we ensure that
> +     value_fetch_lazy can successfully load the component.  */
>    type = value_type (whole);
>    if (NULL != TYPE_DATA_LOCATION (type)
>        && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
>      set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
> +
> +  type = value_type (component);
> +  if (NULL != TYPE_DATA_LOCATION (type)
> +      && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
> +    {
> +      if (VALUE_LVAL (component) == lval_internalvar_component)
> +	{
> +	  gdb_assert (value_lazy (component));
> +	  VALUE_LVAL (component) = lval_memory;
> +	}
> +      else
> +	gdb_assert (VALUE_LVAL (component) == lval_memory);
> +      set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
> +    }

I have a suggestion, but I am not sure it might be right for
everyone, as perhaps other people's brains might be thinking
differently.

In your patch you architected it with one large comment at beginning
followed by a number of conditinal branches, with the comment explaining
the various scenarios that we're about to handle. If your brain works
like mine, I would find the following approach to make it easier for me
to understand the code:

    type = value_type (component);
    if (NULL != TYPE_DATA_LOCATION (type)
        && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST)
      {
        /* COMPONENT's type has a dynamic location, so we need
           to update our component's address to reflect the actual
           location after resolution.  */
        if (VALUE_LVAL (component) == lval_internalvar_component)
          {
            /* This happens when [...].
               We have to change the lval to lval_memory because ... */
            gdb_assert (value_lazy (component));
            VALUE_LVAL (component) = lval_memory;
          }

        /* At this point, we assume that COMPONENT is now an lval_memory,
           and we can now set it address.  */
        gdb_assert (VALUE_LVAL (component) == lval_memory);
        set_value_address (component, TYPE_DATA_LOCATION_ADDR (type));
      }

As I mentioned, maybe you don't think the read code the same way
as I do, and so it would be absolutely fine with me if you don't
agree with the suggestion ;-).

-- 
Joel