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 700035 - Allow registering instance private data during get_type()
Allow registering instance private data during get_type()
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: GProperty
 
 
Reported: 2013-05-09 21:47 UTC by Emmanuele Bassi (:ebassi)
Modified: 2018-05-24 15:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow registering instance private data during get_type() (7.25 KB, patch)
2013-05-09 21:47 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gtype: Try to mop up the private data spillage in the API (11.46 KB, patch)
2013-05-09 23:07 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Deprecate g_type_class_add_private() (1.69 KB, patch)
2013-05-09 23:08 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Document newly added macros and symbols (6.50 KB, patch)
2013-05-10 00:48 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gtype: Fix the instance private size accounting (8.88 KB, patch)
2013-05-13 18:34 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Generate a get_private() function from G_DEFINE_TYPE (6.28 KB, patch)
2013-05-16 14:09 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Test case (2.72 KB, patch)
2013-05-16 16:22 UTC, Alexander Larsson
none Details | Review
Allow registering instance private data during get_type() / v2.0 (16.38 KB, patch)
2013-06-06 13:47 UTC, Emmanuele Bassi (:ebassi)
reviewed Details | Review
Allow registering instance private data during get_type() / v2.1 (16.44 KB, patch)
2013-06-10 23:39 UTC, Emmanuele Bassi (:ebassi)
needs-work Details | Review
Add G_DEFINE_TYPE_WITH_PRIVATE macros (3.15 KB, patch)
2013-06-10 23:39 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Include newly added functions and macros (1.25 KB, patch)
2013-06-10 23:40 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
gparam: Use the new private instance data API (2.33 KB, patch)
2013-06-10 23:40 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
gio: Use the new private instance data declaration (135.12 KB, patch)
2013-06-10 23:40 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Deprecate g_type_class_add_private() (1.51 KB, patch)
2013-06-10 23:43 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Clean up the GObject tutorial a bit (24.74 KB, patch)
2013-06-12 14:20 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Rejig the type definition macros for instance types (3.65 KB, patch)
2013-06-17 18:15 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Add G_PRIVATE_OFFSET (4.80 KB, patch)
2013-06-18 15:15 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Add G_PRIVATE_OFFSET / v2.0 (5.56 KB, patch)
2013-06-18 15:18 UTC, Emmanuele Bassi (:ebassi)
none Details | Review

Description Emmanuele Bassi (:ebassi) 2013-05-09 21:47:11 UTC
Like we do for class private data.

This has a nice side effect of allowing us to write simple macros to be used with G_DEFINE_TYPE and friends, and that we can rely on once we autogenerate accessors through GProperty macros.
Comment 1 Emmanuele Bassi (:ebassi) 2013-05-09 21:47:13 UTC
Created attachment 243741 [details] [review]
Allow registering instance private data during get_type()

There is no actual reason to have the instance private data registration
done at class init time: all g_type_class_add_private() data does with
the class pointer is getting the GType out of it. Registering instance
private data should also automatically return the instance offset for
us.

This allows us to add simple macros to be called inside G_DEFINE_TYPE
and friends.
Comment 2 Allison Karlitskaya (desrt) 2013-05-09 21:48:46 UTC
Dynamic types...
Comment 3 Emmanuele Bassi (:ebassi) 2013-05-09 22:06:07 UTC
< desrt> ebassi: i don't like it
<@ebassi> desrt: heh
< desrt> seriously
< desrt> class unloading/reloading
< desrt> i'm not totally sure i want to kill this
<@ebassi> if you unload you still need to go through get_type, don't you?
<@ebassi> sadly, we cannot inject any code inside class_init
<@ebassi> also, this would only work for static types which cannot be unloaded
< desrt> ebassi: of course we can
<@ebassi> desrt: you want to add _G_DEFINE_TYPE_EXTENDED_BEGIN_WITH_PRIVATE ?
< desrt> ebassi: why do we want private data in _get_type()?
<@ebassi> desrt: because we already have class private data in get_type()?
< desrt> ebassi: that was a mistake, i think :/
<@ebassi> desrt: I think whether or not a type has instance/class private data is an attribute of the type, not of the class_init() function
<@ebassi> desrt: and the code seems to agree with me
< desrt> ebassi: it's an attribute of a particular implementation of that type
<@ebassi> can you have multiple implementations of a type?
< desrt> yes
< desrt> dynamic type loading
< desrt> the new implementation may have a different private size
<@ebassi> eeeehhhh
< desrt> class private data being on the GType is already wrong
<@ebassi> desrt: have we ever tested if the current implementation does even work if we start unloading types with private instance/class data
<@ebassi> desrt: in the middle of a class hierarchy
<@ebassi> well, we really don't have any private data in tests, and we most definitely don't have private data on the dynamic types tests
<@ebassi> so this is pretty much a compatibility minefield already
< desrt> ebassi: it can't be in the middle of a hierarchy
< desrt> the type could not get unloaded unless all subtypes also went away
< desrt> then they'd get reloaded and reinitialised...
<@ebassi> I guess for dynamic types we say that you can only use g_type_class_add_private() and we add a g_type_class_add_class_private() to match; you do have to jump through hoops anyway. for static types, you can use g_type_add_(class|instance)_private()
<@ebassi> we could rename a bunch of functions to make sure that they have static/dynamic in their name
< desrt> ebassi: i guess that would be OK
< desrt> ebassi: would be nice to add explicit g_return_if_fail() on trying to use either of those with a dynamic type, though
<@ebassi> desrt: yeah, that would be good
Comment 4 Emmanuele Bassi (:ebassi) 2013-05-09 23:07:56 UTC
Created attachment 243747 [details] [review]
gtype: Try to mop up the private data spillage in the API

We have a bit of a mess with private data on instances and classes on
dynamic types, so we should try and clean it up a bit.

Static types can register private data structures on instances and
classes directly from the get_type() function; dynamic types cannot, as
their classes may be unloaded and reloaded at will. For this reason, we
should have two families of functions:

  g_type_add_class_private
  g_type_add_instance_private
        - can be called from a get_type() implementation, and only
          work with static types; calling them on dynamic types will
          result in a warning.

  g_type_class_add_class_private
  g_type_class_add_instance_private
        - can only be called from a class_init() implementation, and
          work with both dynamic and static types.

The advantage of the first family is that it's easy to integrate in
the current family of macros for defining GTypes, whereas the latter
requires some work.

The g_type_class_add_instance_private() function replaces
g_type_class_add_private(), which did not conform to the naming scheme,
and returns the private structure offsets, to map to
g_type_add_instance_private().
Comment 5 Emmanuele Bassi (:ebassi) 2013-05-09 23:08:03 UTC
Created attachment 243748 [details] [review]
Deprecate g_type_class_add_private()

This will probably be pretty painful.
Comment 6 Emmanuele Bassi (:ebassi) 2013-05-10 00:48:42 UTC
Created attachment 243752 [details] [review]
docs: Document newly added macros and symbols
Comment 7 Allison Karlitskaya (desrt) 2013-05-10 00:51:30 UTC
I don't like deprecating g_type_class_add_private() just for the purpose of renaming it when it has _so many_ users...  I think that's some pain that maybe we should skip...
Comment 8 Emmanuele Bassi (:ebassi) 2013-05-10 00:57:19 UTC
(In reply to comment #7)
> I don't like deprecating g_type_class_add_private() just for the purpose of
> renaming it when it has _so many_ users...  I think that's some pain that maybe
> we should skip...

yeah, I kind of agree. I attached the commit only for completeness, but I think it's API churn that should be avoided if at all possible.
Comment 9 Emmanuele Bassi (:ebassi) 2013-05-13 08:57:07 UTC
the patches above are not enough: we initialize the type node with a instance.private_size of 0 in type_data_make_W() and update the private_size after class_init(), in case the type adds private data there. this means that, when we register the private data in the get_type() we always get an offset with only the latest class in the hierarchy taken into account.

we should initialize the private offset to a valid value, and warn if, after class_init(), the values do not match.
Comment 10 Emmanuele Bassi (:ebassi) 2013-05-13 18:34:40 UTC
Created attachment 244081 [details] [review]
gtype: Fix the instance private size accounting

We need to copy the private data size early when creating the type data,
but we also need to let classes override it inside the class_init()
implementation, to avoid breaking existing code, and to actually allow
subclasses with private data of classes with private data.

We take a single bit out of the n_preallocs InstanceData field, which
should be unused by now, and we use it to keep a flag that gets set by
the g_type_add_instance_private() function, which we then use as a
guard during class creation.
Comment 11 Emmanuele Bassi (:ebassi) 2013-05-16 14:09:59 UTC
Created attachment 244408 [details] [review]
Generate a get_private() function from G_DEFINE_TYPE

Instead of having a macro with dubious scope, we can tell G_DEFINE_TYPE
and friends to generate a simple inline function that returns a pointer
to the private data structure using the offset. If no private data
structure is defined, the function will never be called, and will be
optimized away by any reasonable compiler. The scope of the function is
also defined to be local to the compilation unit that defined the
private data structure, so we can expect not to leak it to the outside
world.
Comment 12 Alexander Larsson 2013-05-16 14:40:47 UTC
Review of attachment 244081 [details] [review]:

::: gobject/gtype.c
@@ +346,3 @@
   guint16            private_size;
+  guint16            private_size_set : 1;
+  guint16            n_preallocs : 15;

I know this is kinda ununsed, but it wouldn't hurt to add a MAX(info->n_preallocs, 32767) when you set n_preallocs.

@@ +2137,3 @@
+           */
+          if (!node->data->instance.private_size_set)
+            node->data->instance.private_size = pnode->data->instance.private_size;

Isn't this a problem if you mix g_type_class_add_instance_private and g_type_add_instance_private()? Like, what if the parent uses g_type_class_add_instance_private() inbetween type_data_make and here, but the type init used g_type_add_instance_private()?

It seems to me that this makes it problematic to use g_type_add_instance_private() in a subclass, since you don't control if the parent class uses class_add_private.
Comment 13 Emmanuele Bassi (:ebassi) 2013-05-16 15:07:00 UTC
(In reply to comment #12)
> Review of attachment 244081 [details] [review]:
> 
> ::: gobject/gtype.c
> @@ +346,3 @@
>    guint16            private_size;
> +  guint16            private_size_set : 1;
> +  guint16            n_preallocs : 15;
> 
> I know this is kinda ununsed, but it wouldn't hurt to add a
> MAX(info->n_preallocs, 32767) when you set n_preallocs.

good point.

> @@ +2137,3 @@
> +           */
> +          if (!node->data->instance.private_size_set)
> +            node->data->instance.private_size =
> pnode->data->instance.private_size;
> 
> Isn't this a problem if you mix g_type_class_add_instance_private and
> g_type_add_instance_private()? Like, what if the parent uses
> g_type_class_add_instance_private() inbetween type_data_make and here, but the
> type init used g_type_add_instance_private()?

you mean if the parent used g_type_add_instance_private() *and* g_type_class_add_instance_private()? or if the parent uses class_init() and the derived type uses get_type()?

the test suite has a "mixed" case, with the parent using get_type to register the private data, and the derived type using class_init; I also tested the reverse, and a couple of other mixed cases.

> It seems to me that this makes it problematic to use
> g_type_add_instance_private() in a subclass, since you don't control if the
> parent class uses class_add_private.

we always entered the class_init() with a private_size of 0 up until now; the only thing that changed is that we overwrite that field with the new private_size if the class_init() called g_type_class_add_private().

all my tests were successful, but I can add further units.
Comment 14 Alexander Larsson 2013-05-16 16:21:34 UTC
The problem is when you have a class base A which uses g_type_class_add_instance_private(), and a derived class B which uses 
g_type_add_instance_private() *and* we create the type for B before we class_init A.

I'm attaching a testcase addition for this.
Comment 15 Alexander Larsson 2013-05-16 16:22:07 UTC
Created attachment 244431 [details] [review]
Test case
Comment 16 Emmanuele Bassi (:ebassi) 2013-06-06 13:47:55 UTC
Created attachment 246161 [details] [review]
Allow registering instance private data during get_type() / v2.0

reworked after a chat with Ryan on IRC, to follow a new approach: we register the *intent* to have a private data structure inside get_type(), and do the actual registration inside the internal class_init() implementation provided by G_DEFINE_TYPE_EXTENDED and friends.

after we moved all our code to the new idiomatic form to avoid mixed hierarchies, we move the registration to the get_type() implementation, and the class_init() call becomes a no-op.

we'll have to deprecate g_type_class_add_private() to ensure that all code gets gradually ported.
Comment 17 Emmanuele Bassi (:ebassi) 2013-06-06 13:48:45 UTC
for reference, here's the IRC log:

13:58 < desrt> ebassi: so a few more thoughts
13:59 < desrt> ebassi: there is an interesting question about if some day we want to migrate to a world where static types really do actually register their private sizes in _get_type() directly
13:59 < desrt> if you do as i suggest we'll be shooting ourselves in the foot a tiny bit
13:59 < desrt> since we're compiling this macro into everyone's code...
14:00 < desrt> so even if everyone started doign the DEFINE_PRIVATE in the WITH_CODE, we'd still be no better off
14:00 < desrt> because we'd need a flag day for "okay... everyone recompile everything now, k thx"
14:01 < desrt> for this reason, i think calling into gobject might be prudent
14:01 < desrt> another obervation (that i will link, in a moment):
14:01 < desrt> using a separate variable is dumb
14:02 < desrt> we already have the variable that stores the offset
14:02 < desrt> and that is only ever used _after_ class_init
14:02 < desrt> so we could just reuse that variable, storing a positive value there (the size) instead of a negative one (the offset)
14:02 < desrt> so here's my idea:
14:03 < desrt> we proceed exactly as you originally proposed in your first patch, with the new APIs
14:03 < desrt> but...
14:03 < desrt> this new function for adding private data from the _get_type() function.... instead of setting up the private and returning the new offset, it just returns the value you passed in to it
14:04 < desrt> so you end up storing the positive value into this static variable, effectively
14:04 < desrt> then we have some other (very weird) function that takes a reference to an int
14:04 < desrt> and if it's positive, it takes it to be the size of a private that it needs to install, and changes it to the (negative) offset to the private
14:04 < desrt> this function would be private, and only called from the class_init wrapper emitted by G_DEFINE_TYPE
14:05 < desrt> this would do two things:
14:05 < desrt> 1) it would allow us to change this in the future -- the first function could do the registration and the second function could be the no-op
14:05 < desrt> 2) it would allow us to install some intelligence about "it looks like all of my parent classes did their registration statically, so i can safely do mine on-site from get_type()"
14:06 < desrt> so we can do a soft migration as we notice that all parent classes have defined a private from get_type()
14:07 < desrt> under this plan i would expect that we actually do deprecate g_type_class_add_private() btw
14:07 < desrt> to help encourage people to migrate
14:08 < desrt> but i guess we would want to add g_dynamic_type_class_add_private() that still takes a GTypeClass to replace it
14:08 < desrt> that does exactly the same thing as the old function did, but first asserts that we have a dynamic type
Comment 18 Allison Karlitskaya (desrt) 2013-06-06 14:10:42 UTC
Review of attachment 246161 [details] [review]:

This is almost right, although it would have been interesting to see how it would work with the "detect the new world" functionality.

Here are a few alternate ideas, just for completeness:

1) The obvious nul option: forbid people from using the new macros until the library that they are subclassing from has switched.

2) Have some kind of feature in GObject itself to record the sizes of the requested privates and don't allocate them until the stack of classes has been fully initialised.  This is kinda worthless because you'd still need some other function in class_init() to ask "what is my offset, now?" for each class, which is actually pretty much the same as this patch does (except that this patch doesn't gunk up the GObject internals).


So as I see it, this patch is the best option unless we just want to prevent people from using the new get_type()-registered privates until all of their parent classes do as well.

::: gobject/gtype.c
@@ +4569,3 @@
+/* semi-private function, should only be used by G_DEFINE_TYPE_EXTENDED */
+gint
+g_type_class_add_instance_private (gpointer g_class,

Interesting.

I was assuming that this would actually take 'int *' and have a name more like "adjust private offset" and be very very clear that it was only meant to be used internally from the macros.

As it is, this function is an attractive target to use to get rid of those pesky deprecation messages that will pop up now that g_type_class_add_private() is out of fashion.  Which is bad, because it will delay the transition to the Glorious New Future™.

::: gobject/gtype.h
@@ +1549,3 @@
+ * Since: 2.38
+ */
+#define G_ADD_PRIVATE(TypeName, type_name)         { \

Would also be nice to see a G_DEFINE_TYPE_WITH_PRIVATE that takes exactly the same arguments as G_DEFINE_TYPE.  You could use this for the 90% case.

If you want to define an interface then you are already doing WITH_CODE() and you can use G_ADD_PRIVATE() there without much additional effort.
Comment 19 Allison Karlitskaya (desrt) 2013-06-06 14:14:51 UTC
Review of attachment 246161 [details] [review]:

One more comment:

::: gobject/gtype.h
@@ +1572,3 @@
 \
+static inline gpointer \
+type_name##_get_private (TypeName *self) \

What about the case where we want to be able to share the private between multiple .c files via some sort of gtkmenuprivate.h sort of header?  Gtk has literally dozens of such cases...

Also: do you have any ideas about a 'shorter form' like PRIV() or so?  This seems evil...
Comment 20 Emmanuele Bassi (:ebassi) 2013-06-10 23:39:37 UTC
Created attachment 246460 [details] [review]
Allow registering instance private data during get_type() / v2.1

changes since v2.0:

  - g_type_class_add_instance_private() has been changed to use an in-out argument
  - added G_DEFINE_TYPE_WITH_PRIVATE() and G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE() convenience macros
Comment 21 Emmanuele Bassi (:ebassi) 2013-06-10 23:39:56 UTC
Created attachment 246461 [details] [review]
Add G_DEFINE_TYPE_WITH_PRIVATE macros

A simple wrapper around G_DEFINE_TYPE_WITH_CODE and G_ADD_PRIVATE for
convenience of C developers.
Comment 22 Emmanuele Bassi (:ebassi) 2013-06-10 23:40:08 UTC
Created attachment 246462 [details] [review]
docs: Include newly added functions and macros

The functions are private, and an implementation detail of the macros.
Comment 23 Emmanuele Bassi (:ebassi) 2013-06-10 23:40:33 UTC
Created attachment 246463 [details] [review]
gparam: Use the new private instance data API

Since we cannot use G_DEFINE_TYPE_WITH_PRIVATE().
Comment 24 Emmanuele Bassi (:ebassi) 2013-06-10 23:40:51 UTC
Created attachment 246464 [details] [review]
gio: Use the new private instance data declaration

Use the newly added macros, and remove the explicit calls to
g_type_class_add_private().
Comment 25 Emmanuele Bassi (:ebassi) 2013-06-10 23:43:35 UTC
Created attachment 246465 [details] [review]
Deprecate g_type_class_add_private()

this is going to be slightly painful, depending on the code base; porting GIO took me about a couple of hours, because it's a code base of pretty much idiomatic usage of GObject. I expect GTK 3.x to be in the same order of magnitude. GTK 2.x is going to suck, though.
Comment 26 Dan Winship 2013-06-11 12:27:23 UTC
(In reply to comment #25)
> Created an attachment (id=246465) [details] [review]
> Deprecate g_type_class_add_private()
> 
> this is going to be slightly painful

I decided to give GSimpleAsyncResult a one-release reprieve after landing GTask, to give people a little time to port without being complained at...

> GTK 2.x is going to suck, though.

There's no reason GTK 2 needs to be ported. Just have it set GLIB_VERSION_MIN_REQUIRED accordingly.
Comment 27 Emmanuele Bassi (:ebassi) 2013-06-11 13:13:29 UTC
(In reply to comment #26)
> (In reply to comment #25)
> > Created an attachment (id=246465) [details] [review] [details] [review]
> > Deprecate g_type_class_add_private()
> > 
> > this is going to be slightly painful
> 
> I decided to give GSimpleAsyncResult a one-release reprieve after landing
> GTask, to give people a little time to port without being complained at...

I don't particularly object to following the same method. this whole approach is pretty staggered already, anyway: one more release between deprecation and flipping the switch to the Brave New World™ of using macros to install instance private data is not going to hurt anybody.

> > GTK 2.x is going to suck, though.
> 
> There's no reason GTK 2 needs to be ported. Just have it set
> GLIB_VERSION_MIN_REQUIRED accordingly.

it was mostly a data point meant as a comparison for libraries that have lots of classes but few of them with priv pointers in their instance structure (by far the easiest way to port to the new infrastructure). hopefully, they are few and far between, these days.
Comment 28 Emmanuele Bassi (:ebassi) 2013-06-12 14:20:44 UTC
Created attachment 246644 [details] [review]
docs: Clean up the GObject tutorial a bit

Started off by using the new instance private data macro, ended up
cleaning up the obscure, out of date, or simply broken concepts and
paragraphs.
Comment 29 Emmanuele Bassi (:ebassi) 2013-06-17 18:15:42 UTC
Created attachment 247056 [details] [review]
Rejig the type definition macros for instance types

We need to reference the get_type() function from the get_private() one, so the latter has to come after the former. This requires restructuring the type definition macros for instance types — the only ones that can actually have private data

Alex found this when compiling GTK with the wip/gpropert-2 branch, which is based off of this patchset. It's actually worse in Clutter, since we use a bunch of custom types in the test suite.
Comment 30 Alexander Larsson 2013-06-18 10:06:44 UTC
This looks good to me, we should get this in.
Comment 31 Emmanuele Bassi (:ebassi) 2013-06-18 15:15:08 UTC
Created attachment 247166 [details] [review]
Add G_PRIVATE_OFFSET

A macro that evaluates to the offset of a field inside an instance private data structure, similar to G_STRUCT_OFFSET.

This changes the private_offset variable name from type_name_private_offset to TypeName_private_offset, so we can simplify G_ADD_PRIVATE() to only take the camel case type name, and make G_PRIVATE_OFFSET take only two arguments, to preserve the similarity with G_STRUCT_OFFSET.
Comment 32 Emmanuele Bassi (:ebassi) 2013-06-18 15:18:06 UTC
Created attachment 247168 [details] [review]
Add G_PRIVATE_OFFSET / v2.0

wrong commit
Comment 33 Allison Karlitskaya (desrt) 2013-06-19 14:27:47 UTC
Review of attachment 246460 [details] [review]:

::: gobject/gtype.h
@@ +1292,3 @@
                                          GType                       private_type);
+GLIB_AVAILABLE_IN_2_38
+void     g_type_class_add_instance_private (gpointer                 g_class,

the name is still bad.... g_type_class_adjust_private_offset() or something really esoteric sounding would be better.  we don't want people ever using this.
Comment 34 Allison Karlitskaya (desrt) 2013-06-19 14:33:01 UTC
Review of attachment 246460 [details] [review]:

::: gobject/gtype.h
@@ +1515,3 @@
+ *
+ *   G_DEFINE_TYPE_WITH_CODE (MyObject, my_object, G_TYPE_OBJECT,
+ *                            G_ADD_PRIVATE (MyObject, my_object))

also: I find it slightly odd that we give MyObject instead of MyObjectPrivate here, considering this function does nothing at all with MyObject.

We could also about needing 'my_object' with some additional cleverness.  We always declare the private offset integer so we could declare a local variable with a common name as a pointer to this variable and use that pointer from the ADD_PRIVATE macro.  I don't consider this to be any more hacky than the local-variable-of-known-name that gets accessed from G_IMPLEMENT_INTERFACE() already (ie: the gtype).

The only problem is that this may trigger annoying 'local variable assigned but not used' warnings, but I think we could probably work around those.

So, I'd expect to see:

G_DEFINE_TYPE_WITH_CODE (MyObject, my_object, G_TYPE_OBJECT,
                         G_ADD_PRIVATE (MyObjectPrivate))
Comment 35 Allison Karlitskaya (desrt) 2013-06-19 14:33:01 UTC
Review of attachment 246460 [details] [review]:

::: gobject/gtype.h
@@ +1515,3 @@
+ *
+ *   G_DEFINE_TYPE_WITH_CODE (MyObject, my_object, G_TYPE_OBJECT,
+ *                            G_ADD_PRIVATE (MyObject, my_object))

also: I find it slightly odd that we give MyObject instead of MyObjectPrivate here, considering this function does nothing at all with MyObject.

We could also about needing 'my_object' with some additional cleverness.  We always declare the private offset integer so we could declare a local variable with a common name as a pointer to this variable and use that pointer from the ADD_PRIVATE macro.  I don't consider this to be any more hacky than the local-variable-of-known-name that gets accessed from G_IMPLEMENT_INTERFACE() already (ie: the gtype).

The only problem is that this may trigger annoying 'local variable assigned but not used' warnings, but I think we could probably work around those.

So, I'd expect to see:

G_DEFINE_TYPE_WITH_CODE (MyObject, my_object, G_TYPE_OBJECT,
                         G_ADD_PRIVATE (MyObjectPrivate))
Comment 36 Allison Karlitskaya (desrt) 2013-06-19 14:34:29 UTC
Review of attachment 246461 [details] [review]:

::: gobject/gtype.h
@@ +1356,3 @@
+ * Since: 2.38
+ */
+#define G_DEFINE_TYPE_WITH_PRIVATE(TN, t_n, T_P)            G_DEFINE_TYPE_EXTENDED (TN, t_n, T_P, 0, G_ADD_PRIVATE (TN, t_n))

Perfect.

@@ +1399,3 @@
+ * Since: 2.4
+ */
+#define G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE(TN, t_n, T_P)   G_DEFINE_TYPE_EXTENDED (TN, t_n, T_P, G_TYPE_FLAG_ABSTRACT, G_ADD_PRIVATE (TN, t_n))

This one might be a bit too much...  no strong opinion, though.
Comment 37 Allison Karlitskaya (desrt) 2013-06-19 14:42:27 UTC
Review of attachment 246460 [details] [review]:

::: gobject/gtype.h
@@ +1571,3 @@
+type_name##_get_private (TypeName *self) \
+{ \
+  if (G_LIKELY (type_name##_private_offset != 0)) \

Why would this ever be zero?

We really really need this function to always evaluate to exactly one unconditional pointer addition...
Comment 38 Allison Karlitskaya (desrt) 2013-06-19 14:43:57 UTC
Review of attachment 247168 [details] [review]:

::: gobject/gtype.h
@@ +1597,3 @@
+ * Since: 2.38
+ */
+#define G_PRIVATE_OFFSET(TypeName, field) \

I love this.

This provides a nice answer for bug 702563.
Comment 39 Alexander Larsson 2013-06-19 17:12:46 UTC
Review of attachment 246460 [details] [review]:

::: gobject/gtype.h
@@ +1515,3 @@
+ *
+ *   G_DEFINE_TYPE_WITH_CODE (MyObject, my_object, G_TYPE_OBJECT,
+ *                            G_ADD_PRIVATE (MyObject, my_object))

I don't think passing the non-private is weird here. Its less typing, it matches G_PRIVATE_OFFSET and we already have macros depend on the name of the private, so its less typing for no loss.

Also, the later patches change it to G_ADD_PRIVATE(MyObject) which i think is right.

@@ +1571,3 @@
+type_name##_get_private (TypeName *self) \
+{ \
+  if (G_LIKELY (type_name##_private_offset != 0)) \

Yeah, its set from class_intern_init which is before any user code gets called, so i think we should do this unconditionally for performance reasons.
Comment 40 Emmanuele Bassi (:ebassi) 2013-06-19 20:06:48 UTC
new set of (consolidated) patches in the wip/private-rework-3 branch. thanks for the review, much appreciated.

I'm on a hotel wifi, and using MacOS (see above, re: hotel wifi) so no git-bz for me.

changes after the review:

  - consolidated the commits to something slightly more manageable
  - g_type_class_add_instance_private -> g_type_cladd_adjust_private_offset
  - type_name##_get_private() uses only the offset, no g_type_instance_get_private() as a fallback; this removes a bunch of complexity in the macros as well.

(In reply to comment #36)
> Review of attachment 246461 [details] [review]:
> @@ +1399,3 @@
> + * Since: 2.4
> + */
> +#define G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE(TN, t_n, T_P)  
> G_DEFINE_TYPE_EXTENDED (TN, t_n, T_P, G_TYPE_FLAG_ABSTRACT, G_ADD_PRIVATE (TN,
> t_n))
> 
> This one might be a bit too much...  no strong opinion, though.

if you look at the patch porting GIO to the new code, it saves us from changing G_DEFINE_ABSTRACT_TYPE into G_DEFINE_ABSTRACT_TYPE_WITH_CODE + G_ADD_PRIVATE, so I think it should stay, for exactly the same reason why we have G_DEFINE_TYPE_WITH_PRIVATE.
Comment 41 Allison Karlitskaya (desrt) 2013-06-21 14:37:33 UTC
I'm happy with this now.  I see the point of using TypeName instead of TypeNamePrivate in the ADD_PRIVATE() macro now since you use TypeName prefixing to the variable name...

One extra feature that *might* be nice is a G_PRIVATE_FIELD() macro that would expand to an lvalue in the private structure for a class....

  G_PRIVATE_FIELD(MyObject, self, foo)

instead of

  my_object_get_private (self)->foo



although, on balance, it seems that this would actually be more typing...


Might also be nice to declare the get_private() emitted by the macro as const in order to help the compiler a bit in cases like:

  frob (my_object_get_private (self)->foo);
  tweak (my_object_get_private (self)->foo);

so that it can be sure that it is the same 'foo' variable going into both functions (whereas it might have had to entertain the possibility that the private offset variable would change values during the 'frob' call before).

In any case, feel free to land the branch as-is.  Thanks!
Comment 42 Emmanuele Bassi (:ebassi) 2013-06-24 13:16:03 UTC
(In reply to comment #41)
> One extra feature that *might* be nice is a G_PRIVATE_FIELD() macro that would
> expand to an lvalue in the private structure for a class....
> 
>   G_PRIVATE_FIELD(MyObject, self, foo)

I added G_PRIVATE_FIELD_P and G_PRIVATE_FIELD, in the same spirit as G_STRUCT_MEMBER_P and G_STRUCT_MEMBER, respectively. I think they make sense, and they can be useful for quick access.
 
> instead of
> 
>   my_object_get_private (self)->foo
> 
> 
> 
> although, on balance, it seems that this would actually be more typing...

you can't actually do that: the type_name##_get_private() function is defined unconditionally, so if you don't have a TypeName##Private structure, it would result in a compiler error. hence, it returns a gpointer. you could only do:

  ((MyObjectPrivate *) my_object_get_private (self))->foo

but if we have G_PRIVATE_FIELD and G_PRIVATE_FIELD_P then it becomes needless churn.

> Might also be nice to declare the get_private() emitted by the macro as const
> in order to help the compiler a bit in cases like:
> 
>   frob (my_object_get_private (self)->foo);
>   tweak (my_object_get_private (self)->foo);

I'm not sure you can return a gconstpointer without a compiler warning. I tried using the G_GNUC_CONST annotation, but that only works on declarations.
 
> In any case, feel free to land the branch as-is.  Thanks!

cool, thanks.
Comment 43 Emmanuele Bassi (:ebassi) 2013-06-24 13:22:46 UTC
Attachment 246462 [details] pushed as f870d5a - docs: Include newly added functions and macros
Attachment 246463 [details] pushed as aba80ee - gparam: Use the new private instance data API
Attachment 246464 [details] pushed as 32747de - gio: Use the new private instance data declaration
Attachment 246644 [details] pushed as 1f6f7e1 - docs: Clean up the GObject tutorial a bit

pushed to master, except the deprecation of g_type_class_add_private(); I think we should push it once we fix GTK+, to avoid death by a thousand compiler warnings. I'll open a bug against gtk+ and work on the port.
Comment 44 Emmanuele Bassi (:ebassi) 2013-07-26 10:11:17 UTC
gtk has been ported, so I guess we should commit the g_type_class_add_private() deprecation before the Freeze; alternatively, push it as soon as we open the 2.39 development cycle.
Comment 45 Javier Jardón (IRC: jjardon) 2014-01-15 11:57:34 UTC
2.39.x is open, maybe is time to merge the remaining patches?
Comment 46 Emmanuele Bassi (:ebassi) 2014-01-15 12:20:28 UTC
the only remaining patch is the deprecation of g_type_class_add_private(). GLib and GTK+ have been ported already, so I think we can do that.
Comment 47 Allison Karlitskaya (desrt) 2014-01-15 14:42:06 UTC
(In reply to comment #46)
> the only remaining patch is the deprecation of g_type_class_add_private(). GLib
> and GTK+ have been ported already, so I think we can do that.

For a deprecation this large, start of next cycle please...
Comment 48 Emmanuele Bassi (:ebassi) 2017-03-22 10:49:30 UTC
We keep forgetting to deprecate this; I'm going to pick this up for 2.54.
Comment 49 GNOME Infrastructure Team 2018-05-24 15:17:52 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/699.