[RFA,00/12] (somewhat) clean up struct value ownership

Message ID 20180405211507.6103-1-tom@tromey.com
Headers show
Series
  • (somewhat) clean up struct value ownership
Related show

Message

Tom Tromey April 5, 2018, 9:14 p.m.
struct value has long suffered from a complicated approach to
ownership.  In particular, values are reference counted, but they are
also handled specially if they are on the "value chain".  I believe
this has led to bugs on occasion; and anyway requires oddities like
release_value_or_incref.

This series goes some way toward cleaning up ownership for values.  It
introduces a gdb_ref_ptr specialization for values and then changes
various things to use it.  After this, it cleans up various code doing
manual memory mangement related to value; and in particular unifies
the value chain with the reference counting mechanism.

There is still more work that could be done in this area.  For
example:

* struct lval_funcs could be turned into a base class and then the
  implementers rewritten as ordinary objects.

* Likewise struct internalvar_funcs; and this would allow better type
  safety through the removal of union internalvar_data.

* Perhaps the "location" union could be removed from struct value,
  also providing more type safety.

However, I didn't do these, as this series seemed to reach a
reasonable stopping point.

Regression tested by the buildbot.

Tom

Comments

Pedro Alves April 6, 2018, 7:33 p.m. | #1
On 04/05/2018 10:14 PM, Tom Tromey wrote:
> struct value has long suffered from a complicated approach to

> ownership.  In particular, values are reference counted, but they are

> also handled specially if they are on the "value chain".  I believe

> this has led to bugs on occasion; and anyway requires oddities like

> release_value_or_incref.

> 

> This series goes some way toward cleaning up ownership for values.  It

> introduces a gdb_ref_ptr specialization for values and then changes

> various things to use it.  After this, it cleans up various code doing

> manual memory mangement related to value; and in particular unifies

> the value chain with the reference counting mechanism.

> 


This is seriously cool.  Thanks much for doing this.

I've sent a few comments here and there, but overall this looks
good to me.

> There is still more work that could be done in this area.  For

> example:

> 

> * struct lval_funcs could be turned into a base class and then the

>   implementers rewritten as ordinary objects.

> 

> * Likewise struct internalvar_funcs; and this would allow better type

>   safety through the removal of union internalvar_data.

> 

> * Perhaps the "location" union could be removed from struct value,

>   also providing more type safety.

> 


Yeah, this would be nice.

> However, I didn't do these, as this series seemed to reach a

> reasonable stopping point.


Definitely agreed.

As I'm reading the series, I'm also wishing for making some of the
value-related free-functions methods of value instead, so that
instead of having to write ref_ptr::get() calls like in:

 struct type *t = value_type (val.get ());

we'd be able to write instead:

 struct type *t = val->type ();

Thanks,
Pedro Alves
Tom Tromey April 6, 2018, 9:20 p.m. | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> As I'm reading the series, I'm also wishing for making some of the
Pedro> value-related free-functions methods of value instead

Yeah.  When I wrote my note I meant to mention a couple of other things,
and then forgot to.  Class-ifying value more completely was one of them.
Another one would be to do a bigger transform from "struct value *" to
"value_ref_ptr" (and "const value_ref_ptr &") everywhere, to try to
reduce the number of inc/dec-ref calls to a bare minimum, maybe even zero.

I'm not sure if I'll attempt those, since I have other maintenance
cleanups in mind as well.

Tom
Tom Tromey April 6, 2018, 9:44 p.m. | #3
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> I've sent a few comments here and there, but overall this looks
Pedro> good to me.

Since the requested changes were minor, I'm going to check it in.

Tom
Tom Tromey April 8, 2018, 9:32 p.m. | #4
Pedro> As I'm reading the series, I'm also wishing for making some of the
Pedro> value-related free-functions methods of value instead, so that
Pedro> instead of having to write ref_ptr::get() calls like in:

I did some of this today, but it's hard to know where to stop.

I was thinking maybe just the things requiring direct access to the
members; and then anything else could remain a free function.  I'm not
sure though and there might be exceptions to that.

Tom