GNOME Bugzilla – Bug 521707
Class private data
Last modified: 2011-02-18 15:59:31 UTC
Hi, it would be nice if GObject would provide something like g_type_class_add_private(), but for the class structs. It could be called g_type_class_add_class_private(g_type, gsize private_size), must be called somewhere in _get_type() after type registration similar to interfaces and would get a G_TYPE_CLASS_GET_PRIVATE(g_class, g_type, c_type) macro for getting the private data. The private data would be allocated together with the class struct (also for subclasses) and for subclasses would be memcpy'd from the parent classes (like the class structs). The classes are responsible for doing whatever is necessary to keep the private data valid for subclasses in base_init().
What is the use case for this ?
See [0] for example. GstBaseTransformPrivate (which is instance private) has a member that is in reality a class property (gap_aware). As GstBaseTransformClass is already filled up it had to be added to the instance private data instead. So, the use case for this is all private data that is class specific. It's nothing very common but still happens often enough to add the class private data magic... also implementing this shouldn't be too hard, I'll take a look at it next week :) [0] http://webcvs.freedesktop.org/gstreamer/gstreamer/libs/gst/base/gstbasetransform.c?revision=1.107&view=markup
(In reply to comment #1) > What is the use case for this ? This is also usefull for hiding GSEAL()-ed class data.
Tim, did you start working on this already and will it be in 2.20?
(In reply to comment #4) > Tim, did you start working on this already and will it be in 2.20? I haven't started work on this yet, and am still busy with other stuff for a while. So if anyone would cook up a patch here, that could help.
I'm working on this now and already have a working, half-finished patch...
Created attachment 125921 [details] [review] gtype-class-private.diff This works fine for me and even is valgrind clean ;) It might make sense to use something else than a guint16 for the class private size but this was used for the instance private size too. Any comments? :)
Tim, could you take a look at this? Would be nice to get it into 2.20 :)
ping?
ping²? Maybe we can get this into 2.22 at least...
This could also be useful for adding new vfunc/signal slots when class structs are full couldn't it?
(In reply to comment #11) > This could also be useful for adding new vfunc/signal slots when class structs > are full couldn't it? Sure, but you would need functions to override them then as a subclass shouldn't read private data I guess.
...and another ping. Does someone know when Tim will be available again? According to Matthias he's the only one who should review GObject patches and there are a lot interesting patches for GObject in Bugzilla that would be nice to have at least in 2.21/2.22...
Comment on attachment 125921 [details] [review] gtype-class-private.diff Trying to pre-review this patch. Please note that I am not at all experienced with GObject. Nevertheless I will try to give a useful comment or two below ;) In general the patch does make sense to me. I will also help pinging Tim to get this reviewed, because we want to have this for GTK+ 2.90/3.0. >@@ -1889,7 +1901,10 @@ > !node->data->class.class && > node->data->class.init_state == UNINITIALIZED); > >- class = g_malloc0 (node->data->class.class_size); >+ if (node->data->class.class_private_size) >+ class = g_malloc0 (ALIGN_STRUCT (node->data->class.class_size) + node->data->class.class_private_size); >+ else >+ class = g_malloc0 (node->data->class.class_size); > node->data->class.class = class; > node->data->class.init_state = BASE_CLASS_INIT; What I do not fully understand is that you are only taking class.class_private_size into account here. Am I right in observing that for instantiatable types, only instance.class_private_size is set and that that value should also be considered here? Maybe split this out in a separate inline function like is done for type_total_instance_size_I ()? >+ G_WRITE_LOCK (&type_rw_lock); >+ >+ if (node->is_instantiatable) { >+ offset = ALIGN_STRUCT (node->data->instance.class_private_size); >+ node->data->instance.class_private_size = offset + private_size; >+ } else { >+ offset = ALIGN_STRUCT (node->data->class.class_private_size); >+ node->data->class.class_private_size = offset + private_size; >+ } This chunk needs to have the indentation fixed.
(In reply to comment #14) > (From update of attachment 125921 [details] [review]) > Trying to pre-review this patch. Please note that I am not at all experienced > with GObject. Nevertheless I will try to give a useful comment or two below ;) > > In general the patch does make sense to me. I will also help pinging Tim to > get this reviewed, because we want to have this for GTK+ 2.90/3.0. Thanks :) > >@@ -1889,7 +1901,10 @@ > > !node->data->class.class && > > node->data->class.init_state == UNINITIALIZED); > > > >- class = g_malloc0 (node->data->class.class_size); > >+ if (node->data->class.class_private_size) > >+ class = g_malloc0 (ALIGN_STRUCT (node->data->class.class_size) + node->data->class.class_private_size); > >+ else > >+ class = g_malloc0 (node->data->class.class_size); > > node->data->class.class = class; > > node->data->class.init_state = BASE_CLASS_INIT; > > What I do not fully understand is that you are only taking > class.class_private_size into account here. Am I right in observing that for > instantiatable types, only instance.class_private_size is set and that that > value should also be considered here? Yes, but as data is an union and the class and instance members are identical for sizeof(class) this works nonetheless ;) But you're right, it would be better to not implicitely assume this (OTOH the existing code does that too by only accessing the class part in any case). It might make sense to change struct _InstanceData to struct _InstanceData { ClassData common; guint16 instance_size; ... }; instead so this is not implicit anymore. What do you think? > >+ G_WRITE_LOCK (&type_rw_lock); > >+ > >+ if (node->is_instantiatable) { > >+ offset = ALIGN_STRUCT (node->data->instance.class_private_size); > >+ node->data->instance.class_private_size = offset + private_size; > >+ } else { > >+ offset = ALIGN_STRUCT (node->data->class.class_private_size); > >+ node->data->class.class_private_size = offset + private_size; > >+ } > > This chunk needs to have the indentation fixed. Right, but that can be made when this can finally be committed. It's only a matter of adding some \n here and there ;)
(In reply to comment #15) > > What I do not fully understand is that you are only taking > > class.class_private_size into account here. Am I right in observing that for > > instantiatable types, only instance.class_private_size is set and that that > > value should also be considered here? > > Yes, but as data is an union and the class and instance members are identical > for sizeof(class) this works nonetheless ;) But you're right, it would be > better to not implicitely assume this (OTOH the existing code does that too by > only accessing the class part in any case). Oh! Tricky :) But yea, if the existing code does it as well, why not. What I then still don't get is why the "class_private_size" field in InstanceData does not immediately follow the "gpointer class" field like it does in ClassData. > It might make sense to change struct _InstanceData to > > struct _InstanceData > { > ClassData common; > guint16 instance_size; > ... > }; > > instead so this is not implicit anymore. What do you think? That would really be something for Tim to decide. It does require typing another .common. for all the field accesses ... > Right, but that can be made when this can finally be committed. It's only a > matter of adding some \n here and there ;) Yes, of course :)
(In reply to comment #16) > (In reply to comment #15) > > > What I do not fully understand is that you are only taking > > > class.class_private_size into account here. Am I right in observing that for > > > instantiatable types, only instance.class_private_size is set and that that > > > value should also be considered here? > > > > Yes, but as data is an union and the class and instance members are identical > > for sizeof(class) this works nonetheless ;) But you're right, it would be > > better to not implicitely assume this (OTOH the existing code does that too by > > only accessing the class part in any case). > > Oh! Tricky :) But yea, if the existing code does it as well, why not. What I > then still don't get is why the "class_private_size" field in InstanceData does > not immediately follow the "gpointer class" field like it does in ClassData. Because that's a bug :) I wonder why it worked in my tests then... I guess it should be moved at the same position for consistency reasons anyway. I'll read through the code again later, everything I said so far was from memory and could be wrong ;)
Created attachment 141221 [details] [review] class-private.diff Fixed all previously mentioned problems and added a comment for the class/instance union stuff...
Bug #592942 contains a patch for vala to use g_type_class_add_private() etc to store class private fields when valac gets --target-glib=2.22 passed. Without this it will use some workaround code that essentially does the same for hiding the class private data but in a much less clean way. This can be used for testing the patch
Created attachment 142427 [details] test.c Small sample application that has two GObject types, Foo and Bar, where Bar is a subclass of Foo. Foo implements two instance counters, one global counter that counts the number of instances of Foo and Bar together. And a type specific counter that only counts the number of instances of a single specific type. The latter is implemented as class private field. (vala sources for this will be attached next to this)
Created attachment 142428 [details] test.vala I'm attaching this example so people can easily see how the attached patch can be used and that it really works ;)
...and another GLib release. Maybe it can be included in 2.24 then?
Kris/Tim: Can this get a review please?
(In reply to comment #23) > Kris/Tim: Can this get a review please? As I said in the bug I did a "pre-review". I cannot do more than that, I am not a glib maintainer and do not have enough experience with the GObject internals to really approve the patch for inclusion.
Tim: can we get this into 2.24 please? :) As you said, it might be nice to have for GTK+3 to hide class fields properly...
+ if (G_UNLIKELY ((private_node->is_instantiatable && private_node->data->instance.class_private_size == parent_node->data->instance.class_private_size) || + (!private_node->is_instantiatable && private_node->is_classed && private_node->data->class.class_private_size == parent_node->data->class.class_private_size))) These checks are double, since is_instantiable implies is_classed. Why not just test is_classed. Same here: + if (private_node->is_instantiatable) + offset = ALIGN_STRUCT (class_node->data->instance.class_size); + else + offset = ALIGN_STRUCT (class_node->data->class.class_size); Otherwise this looks good to me.
Created attachment 156356 [details] [review] 0001-Bug-612502-Add-support-for-class-private-data.patch Thanks, I've simplified those checks now. is_classed is already checked in both cases in the beginning. Anything else needed before it can be pushed? :)
Comment on attachment 156356 [details] [review] 0001-Bug-612502-Add-support-for-class-private-data.patch Ok, lets get this in then.
commit 41383b303c0bb54da68bbf5500b5e2d9e552ab69 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Wed Mar 17 15:11:00 2010 +0100 Bug 612502 - Add support for class private data This adds the two new functions g_type_add_class_private() and g_type_class_get_private() and a convenience macro for the getter G_TYPE_CLASS_GET_PRIVATE().
Comment on attachment 156356 [details] [review] 0001-Bug-612502-Add-support-for-class-private-data.patch Thanks, I've pushed this now :)
\o/