Simpify varobj children handling for C++

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

Simpify varobj children handling for C++

Vladimir Prus

This patch merges the code for getting name, value and type of
varobj children for C++ into one function, just like the previously
posted patch for C.

This was manually tested from KDevelop. We really need tests
for MI C++ functionality, and I plan to write them, but only
after my patch that makes it possible to write MI tests using
just C files is approved.

Is this patch OK provided that tests will be writted before committing?

- Volodya

        Refactor getting children name, value and type access
        for varobjs in C++.
        * value.c (value_as_address): Use coerce_array_proper
        instead of coerce_array so that not fail for references.
        (coerce_array_proper): New function.
        (coerce_array): Use the above.
        * value.h (coerce_array_proper): Declare.
        * valops.c (value_ind): Handle references.
        * varobj.c (adjust_value_for_children_access): New.
        (c_describe_child): Use the above.
        (enum accessibility): New.
        (match_accessibility): New function.
        (cplus_describe_child): New function.
        (cplus_name_of_child, cplus_value_of_child)
        (cplus_type_of_child): Reimplement in terms
        of cplus_describe_child.

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

Re: Simpify varobj children handling for C++

Vladimir Prus
Vladimir Prus wrote:

>
> This patch merges the code for getting name, value and type of
> varobj children for C++ into one function, just like the previously
> posted patch for C.
>
> This was manually tested from KDevelop. We really need tests
> for MI C++ functionality, and I plan to write them, but only
> after my patch that makes it possible to write MI tests using
> just C files is approved.
>
> Is this patch OK provided that tests will be writted before committing?
Here's a revised version of this. I've noticed that the new
adjust_value_for_children_access function subsumes get_type_deref,
and therefore this patch removes get_type_deref.

- Volodya

        Refactor getting children name, value and type access
        for varobjs in C++.
        * value.c (value_as_address): Use coerce_array_proper
        instead of coerce_array so that not fail for references.
        (coerce_array_proper): New function.
        (coerce_array): Use the above.
        * value.h (coerce_array_proper): Declare.
        * valops.c (value_ind): Handle references.
        * varobj.c (get_type_deref): Remove.
        (adjust_value_for_children_access): New.
        (c_number_of_children): Use the above.
        (c_describe_child): Likewise.
        (enum accessibility): New.
        (match_accessibility): New function.
        (cplus_describe_child): New function.
        (cplus_name_of_child, cplus_value_of_child)
        (cplus_type_of_child): Reimplement in terms
        of cplus_describe_child.
        (cplus_number_of_children): Use
        adjust_value_for_children_access.

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

Re: Simpify varobj children handling for C++

Daniel Jacobowitz-2
On Mon, Dec 25, 2006 at 11:16:15AM +0300, Vladimir Prus wrote:
>         * value.c (value_as_address): Use coerce_array_proper
>         instead of coerce_array so that not fail for references.
>         (coerce_array_proper): New function.
>         (coerce_array): Use the above.

I definitely need more information on this.  What fails beforehand,
and how did you get there?

> @@ -950,7 +950,8 @@ value_ind (struct value *arg1)
>    if (TYPE_CODE (base_type) == TYPE_CODE_INT)
>      return value_at_lazy (builtin_type_int,
>    (CORE_ADDR) value_as_long (arg1));
> -  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR)
> +  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR
> +   || TYPE_CODE (base_type) == TYPE_CODE_PTR)
>      {
>        struct type *enc_type;
>        /* We may be pointing to something embedded in a larger object */

Something tells me you didn't actually need value_ind to handle
references... :-)

> +/* Given a value and a type of a variable object,

"the value and the type of a variable object", because
the value is the property of some specific variable object.

> +   adjust those value and type to those necessary

"adjust the"

> +   for getting childrens of the variable object.

"children"

> +   This includes dereferencing top-level reference
> +   to all types and dereferencing pointers to
> +   structures.  

"references" to match "all types"

> +static void
> +adjust_value_for_children_access (struct value **value,
> +  struct type **type)

"adjust_value_for_child_access", also.

> +{
> +  gdb_assert (type && *type);
> +
> +  *type = check_typedef (*type);
> +  
> +  /* If the parent is reference, we always strip the
> +     reference when getting children, since in C++,
> +     reference is basically undistinguishable in
> +     usage from a plain variable.  */

"is a reference", "in C++, a reference".

> +  /* The 'get_target_type' function call check_typedef on

"calls"

> @@ -1876,6 +1886,10 @@ c_describe_child (struct varobj *parent,
>        if (cvalue && value)
>   gdb_value_ind (value, cvalue);
>  
> +      /* The get_target_type function calls check_typedef
> + on the result.  I'm not sure if showing check_typedefed
> + type for the child as opposed to the declared type is
> + right.  */
>        if (ctype)
>   *ctype = get_target_type (type);
>        

It'd be nice if we didn't do that.  We go to some effort to show
typedefs in the CLI for ptype.

> +  /* This is baseclass.  */

"is a"


--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: Simpify varobj children handling for C++

Vladimir Prus
Daniel Jacobowitz wrote:

> On Mon, Dec 25, 2006 at 11:16:15AM +0300, Vladimir Prus wrote:
>>         * value.c (value_as_address): Use coerce_array_proper
>>         instead of coerce_array so that not fail for references.
>>         (coerce_array_proper): New function.
>>         (coerce_array): Use the above.
>
> I definitely need more information on this.  What fails beforehand,
> and how did you get there?

In truth, this might not be needed. Previously I found that
value_ind just errors out on references and tried to fix this
in this patch.

>> @@ -950,7 +950,8 @@ value_ind (struct value *arg1)
>>    if (TYPE_CODE (base_type) == TYPE_CODE_INT)
>>      return value_at_lazy (builtin_type_int,
>>  (CORE_ADDR) value_as_long (arg1));
>> -  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR)
>> +  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR
>> +       || TYPE_CODE (base_type) == TYPE_CODE_PTR)
>>      {
>>        struct type *enc_type;
>>        /* We may be pointing to something embedded in a larger object */
>
> Something tells me you didn't actually need value_ind to handle
> references... :-)
Why? Because that's a bad idea or because varobj code does not
need to dereference rereferences? The latter is true,
attached version of the patch builds on the
"fix 'editable' attribute for references" patch I've posted earlier
and now 'adjust_value_for_child_access' asserts that the type
is not reference.
 

>> @@ -1876,6 +1886,10 @@ c_describe_child (struct varobj *parent,
>>        if (cvalue && value)
>>  gdb_value_ind (value, cvalue);
>>  
>> +      /* The get_target_type function calls check_typedef
>> +     on the result.  I'm not sure if showing check_typedefed
>> +     type for the child as opposed to the declared type is
>> +     right.  */
>>        if (ctype)
>>  *ctype = get_target_type (type);
>>        
>
> It'd be nice if we didn't do that.  We go to some effort to show
> typedefs in the CLI for ptype.
Changed. Revised patch attached, OK?

- Volodya

        Refactor getting children name, value and type access
        for varobjs in C++.
        * varobj.c (get_type_deref): Remove.
        (adjust_value_for_child_access): New.
        (c_number_of_children): Use the above.
        (c_describe_child): Likewise.
        (enum accessibility): New.
        (match_accessibility): New function.
        (cplus_describe_child): New function.
        (cplus_name_of_child, cplus_value_of_child)
        (cplus_type_of_child): Reimplement in terms
        of cplus_describe_child.
        (cplus_number_of_children): Use
        adjust_value_for_child_access.

delta.diff (7K) Download Attachment
path_4_unify_cpp__gdb_mainline.diff (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Simpify varobj children handling for C++

Daniel Jacobowitz-2
On Wed, Jan 17, 2007 at 09:55:53PM +0300, Vladimir Prus wrote:

> >> @@ -950,7 +950,8 @@ value_ind (struct value *arg1)
> >>    if (TYPE_CODE (base_type) == TYPE_CODE_INT)
> >>      return value_at_lazy (builtin_type_int,
> >>  (CORE_ADDR) value_as_long (arg1));
> >> -  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR)
> >> +  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR
> >> +       || TYPE_CODE (base_type) == TYPE_CODE_PTR)
> >>      {
> >>        struct type *enc_type;
> >>        /* We may be pointing to something embedded in a larger object */
> >
> > Something tells me you didn't actually need value_ind to handle
> > references... :-)
>
> Why? Because that's a bad idea or because varobj code does not
> need to dereference rereferences? The latter is true,
> attached version of the patch builds on the
> "fix 'editable' attribute for references" patch I've posted earlier
> and now 'adjust_value_for_child_access' asserts that the type
> is not reference.

All I meant was that you added "is TYPE_CODE_PTR or is TYPE_CODE_PTR".
I guess you meant to check TYPE_CODE_REF, but you must not have needed
it if you didn't notice the typo.

> +get_value_type (struct varobj *var)
>  {
>    struct type *type;
>  
> -  type = get_type (var);
> +  if (var->value)
> +    type = value_type (var->value);
> +  else
> +    type = var->type;
>  
> -  if (type)
> -    {
> -      if (TYPE_CODE (type) == TYPE_CODE_REF)
> - type = get_target_type (type);
> -      if (TYPE_CODE (type) == TYPE_CODE_PTR)
> - type = get_target_type (type);
> -    }
> +  if (TYPE_CODE (type) == TYPE_CODE_REF)
> +    type = get_target_type (type);
> +
> +  type = check_typedef (type);
>  
>    return type;
>  }

I think that if you want to check for references, you need to call
check_typedef first (I just checked that g++ does allow typedefs
to reference types).  Otherwise this looks OK to commit.

It would be nice to have some tests for not stripping typedefs,
if that's practical, but it's not really important.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: Simpify varobj children handling for C++

Vladimir Prus
Daniel Jacobowitz wrote:

> On Wed, Jan 17, 2007 at 09:55:53PM +0300, Vladimir Prus wrote:
>> >> @@ -950,7 +950,8 @@ value_ind (struct value *arg1)
>> >>    if (TYPE_CODE (base_type) == TYPE_CODE_INT)
>> >>      return value_at_lazy (builtin_type_int,
>> >>  (CORE_ADDR) value_as_long (arg1));
>> >> -  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR)
>> >> +  else if (TYPE_CODE (base_type) == TYPE_CODE_PTR
>> >> +       || TYPE_CODE (base_type) == TYPE_CODE_PTR)
>> >>      {
>> >>        struct type *enc_type;
>> >>        /* We may be pointing to something embedded in a larger object
>> >>        */
>> >
>> > Something tells me you didn't actually need value_ind to handle
>> > references... :-)
>>
>> Why? Because that's a bad idea or because varobj code does not
>> need to dereference rereferences? The latter is true,
>> attached version of the patch builds on the
>> "fix 'editable' attribute for references" patch I've posted earlier
>> and now 'adjust_value_for_child_access' asserts that the type
>> is not reference.
>
> All I meant was that you added "is TYPE_CODE_PTR or is TYPE_CODE_PTR".
> I guess you meant to check TYPE_CODE_REF, but you must not have needed
> it if you didn't notice the typo.
Ah ;-)

>> +get_value_type (struct varobj *var)
>>  {
>>    struct type *type;
>>  
>> -  type = get_type (var);
>> +  if (var->value)
>> +    type = value_type (var->value);
>> +  else
>> +    type = var->type;
>>  
>> -  if (type)
>> -    {
>> -      if (TYPE_CODE (type) == TYPE_CODE_REF)
>> -    type = get_target_type (type);
>> -      if (TYPE_CODE (type) == TYPE_CODE_PTR)
>> -    type = get_target_type (type);
>> -    }
>> +  if (TYPE_CODE (type) == TYPE_CODE_REF)
>> +    type = get_target_type (type);
>> +
>> +  type = check_typedef (type);
>>  
>>    return type;
>>  }
>
> I think that if you want to check for references, you need to call
> check_typedef first (I just checked that g++ does allow typedefs
> to reference types).  Otherwise this looks OK to commit.
The check_typedef added in "editable fix" patch, that's checked in.

The varobj children patch for C++ proper that I've just checked in as
well is attached.

>
> It would be nice to have some tests for not stripping typedefs,
> if that's practical, but it's not really important.

It would be good, but writing comprehensive tests is not a quick task.
It's on my todo, but definitely not today.

- Volodya

 

path_4_unify_cpp_as_committed.diff (16K) Download Attachment