After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 521707 - Class private data
Class private data
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal enhancement
: ---
Assigned To: Tim Janik
gtkdev
Depends on:
Blocks: 592942
 
 
Reported: 2008-03-11 04:40 UTC by Sebastian Dröge (slomo)
Modified: 2011-02-18 15:59 UTC
See Also:
GNOME target: 2.30.x
GNOME version: ---


Attachments
gtype-class-private.diff (7.98 KB, patch)
2009-01-07 11:20 UTC, Sebastian Dröge (slomo)
none Details | Review
class-private.diff (8.77 KB, patch)
2009-08-20 08:54 UTC, Sebastian Dröge (slomo)
reviewed Details | Review
test.c (6.71 KB, text/x-csrc)
2009-09-03 18:04 UTC, Sebastian Dröge (slomo)
  Details
test.vala (649 bytes, text/x-vala)
2009-09-03 18:05 UTC, Sebastian Dröge (slomo)
  Details
0001-Bug-612502-Add-support-for-class-private-data.patch (8.60 KB, patch)
2010-03-17 14:13 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2008-03-11 04:40:07 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().
Comment 1 Matthias Clasen 2008-03-16 03:57:16 UTC
What is the use case for this ?
Comment 2 Sebastian Dröge (slomo) 2008-03-16 10:17:58 UTC
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
Comment 3 Tim Janik 2008-08-18 12:41:33 UTC
(In reply to comment #1)
> What is the use case for this ?

This is also usefull for hiding GSEAL()-ed class data.
Comment 4 Sebastian Dröge (slomo) 2008-12-17 08:13:11 UTC
Tim, did you start working on this already and will it be in 2.20?
Comment 5 Tim Janik 2008-12-17 09:21:20 UTC
(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.
Comment 6 Sebastian Dröge (slomo) 2009-01-06 19:27:54 UTC
I'm working on this now and already have a working, half-finished patch...
Comment 7 Sebastian Dröge (slomo) 2009-01-07 11:20:45 UTC
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? :)
Comment 8 Sebastian Dröge (slomo) 2009-01-25 08:36:51 UTC
Tim, could you take a look at this? Would be nice to get it into 2.20 :)
Comment 9 Sebastian Dröge (slomo) 2009-02-14 13:18:01 UTC
ping?
Comment 10 Sebastian Dröge (slomo) 2009-05-04 07:03:22 UTC
ping²? Maybe we can get this into 2.22 at least...
Comment 11 Cody Russell 2009-05-23 00:26:31 UTC
This could also be useful for adding new vfunc/signal slots when class structs are full couldn't it?
Comment 12 Sebastian Dröge (slomo) 2009-05-23 08:02:41 UTC
(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.
Comment 13 Sebastian Dröge (slomo) 2009-05-29 08:44:41 UTC
...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 14 Kristian Rietveld 2009-08-19 20:21:46 UTC
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.
Comment 15 Sebastian Dröge (slomo) 2009-08-20 05:53:33 UTC
(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 ;)
Comment 16 Kristian Rietveld 2009-08-20 06:21:16 UTC
(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 :)
Comment 17 Sebastian Dröge (slomo) 2009-08-20 07:36:54 UTC
(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 ;)
Comment 18 Sebastian Dröge (slomo) 2009-08-20 08:54:01 UTC
Created attachment 141221 [details] [review]
class-private.diff

Fixed all previously mentioned problems and added a comment for the class/instance union stuff...
Comment 19 Sebastian Dröge (slomo) 2009-08-24 19:17:42 UTC
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
Comment 20 Sebastian Dröge (slomo) 2009-09-03 18:04:37 UTC
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)
Comment 21 Sebastian Dröge (slomo) 2009-09-03 18:05:17 UTC
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 ;)
Comment 22 Sebastian Dröge (slomo) 2009-09-23 10:08:21 UTC
...and another GLib release. Maybe it can be included in 2.24 then?
Comment 23 André Klapper 2009-11-12 16:54:53 UTC
Kris/Tim: Can this get a review please?
Comment 24 Kristian Rietveld 2009-11-12 19:41:01 UTC
(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.
Comment 25 Sebastian Dröge (slomo) 2010-01-15 09:06:07 UTC
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...
Comment 26 Alexander Larsson 2010-03-17 13:34:42 UTC
+      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.
Comment 27 Sebastian Dröge (slomo) 2010-03-17 14:13:23 UTC
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 28 Matthias Clasen 2010-03-17 17:13:55 UTC
Comment on attachment 156356 [details] [review]
0001-Bug-612502-Add-support-for-class-private-data.patch

Ok, lets get this in then.
Comment 29 Sebastian Dröge (slomo) 2010-03-17 17:17:36 UTC
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 30 Sebastian Dröge (slomo) 2010-03-17 17:17:45 UTC
Comment on attachment 156356 [details] [review]
0001-Bug-612502-Add-support-for-class-private-data.patch

Thanks, I've pushed this now :)
Comment 31 Marc-Andre Lureau 2010-03-17 17:21:41 UTC
\o/