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 648526 - (GProperty) new Property and Accessor declaration
(GProperty)
new Property and Accessor declaration
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on: 700035
Blocks: 536939 634793 703671 703672
 
 
Reported: 2011-04-23 21:58 UTC by Emmanuele Bassi (:ebassi)
Modified: 2018-05-24 13:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
initial version of the GProperty API contract (22.34 KB, text/plain)
2011-04-23 21:58 UTC, Emmanuele Bassi (:ebassi)
  Details
example of GProperty usage (2.91 KB, text/plain)
2011-04-23 22:00 UTC, Emmanuele Bassi (:ebassi)
  Details
GProperty API contract / v2.0 (33.00 KB, text/plain)
2011-04-30 23:59 UTC, Emmanuele Bassi (:ebassi)
  Details
gobject: Add GProperty / v0.1 (72.11 KB, patch)
2011-05-05 16:54 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add GProperty / v0.2 (140.57 KB, patch)
2011-05-10 17:41 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add GProperty / v0.3 (217.06 KB, patch)
2011-05-16 14:36 UTC, Emmanuele Bassi (:ebassi)
reviewed Details | Review
gobject: Add GProperty / v0.4 (225.85 KB, patch)
2011-05-17 13:01 UTC, Emmanuele Bassi (:ebassi)
reviewed Details | Review
perf: Compare GParamSpec and GProperty usage (12.89 KB, patch)
2011-05-17 13:03 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add override_property_default() (5.81 KB, patch)
2011-05-17 16:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Do not box values in g_property_get_range() (5.37 KB, patch)
2011-05-21 08:56 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Pack the GProperty structure (18.40 KB, patch)
2011-05-21 08:56 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add GProperty / v3.0 (171.71 KB, patch)
2013-06-11 13:25 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Add default values (18.91 KB, patch)
2013-06-11 13:26 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Use GProperty varargs API when accessing properties (7.68 KB, patch)
2013-06-11 13:26 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Add autogeneration of accessors (14.59 KB, patch)
2013-06-11 13:27 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gparam: Add setters for static nick and blurb (2.68 KB, patch)
2013-06-11 13:27 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Add macros for defining properties (6.21 KB, patch)
2013-06-11 13:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add quark-based locking (3.03 KB, patch)
2013-06-11 13:29 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Simplify code generation and private data access (23.02 KB, patch)
2013-06-11 13:29 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Initialize GProperties at instance init time (6.45 KB, patch)
2013-06-11 13:31 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add GProperty / v3.1 (163.98 KB, patch)
2013-06-13 17:47 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Add default values (18.73 KB, patch)
2013-06-13 17:47 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Use GProperty varargs API when accessing properties (7.68 KB, patch)
2013-06-13 17:48 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Add autogeneration of accessors (14.59 KB, patch)
2013-06-13 17:48 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gparam: Add setters for static nick and blurb (2.68 KB, patch)
2013-06-13 17:48 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Initialize GProperties at instance init time (6.46 KB, patch)
2013-06-13 17:49 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Add macros for defining properties (27.58 KB, patch)
2013-06-13 17:49 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Add macros for defining properties (32.00 KB, patch)
2013-06-14 15:01 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperties: Rename properties static variable to avoid collisions (3.26 KB, patch)
2013-06-18 09:37 UTC, Alexander Larsson
none Details | Review
gobject: Add GProperty / v3.2 (162.31 KB, patch)
2013-06-24 13:25 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gproperty: Add default values (19.01 KB, patch)
2013-06-24 13:26 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Use GProperty varargs API when accessing properties (7.68 KB, patch)
2013-06-24 13:26 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Initialize GProperties at instance creation (7.36 KB, patch)
2013-06-24 13:26 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gparam: Add setters for static nick and blurb (2.68 KB, patch)
2013-06-24 13:26 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Add autogeneration of accessors (14.80 KB, patch)
2013-06-24 13:27 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Add macros for defining properties (28.89 KB, patch)
2013-06-24 13:27 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
property: Use the field offset everywhere (14.78 KB, patch)
2013-06-24 13:27 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
gobject: Add wrappers for overriding GProperty default values (8.90 KB, patch)
2013-06-24 13:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Update examples in GProperty (7.81 KB, patch)
2013-06-24 13:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Add a section on the GProperty macros (2.65 KB, patch)
2013-06-24 13:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Update the GObject tutorial section on properties (17.92 KB, patch)
2013-06-24 13:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review

Description Emmanuele Bassi (:ebassi) 2011-04-23 21:58:54 UTC
Created attachment 186534 [details]
initial version of the GProperty API contract

the current property definition is a bit on the verbose side; plus, it comes in so many ways, shapes and forms, that creating a parser for introspection boils down to parsing C code.

whilst there's an option for an IDL (of various stages of complexity) we can probably still squeeze something out of the C pre-processor to have a stable and simpler syntax for property/accessor definition.

taking a page from the current GObject, we could add new macros for:

  - declaring and installing properties
  - declaring and generating accessors

and we can also fix a long-standing issue of having accessor functions decoupled from the properties they expose, instead of being tightly coupled.

I'm attaching a first draft of an API contract for GProperty, a GParamSpec sub-class that takes over all the sub-types of GParamSpec into a single type (with different constructors and "decorators" for type-specific modifiers, e.g. ranges and default values). the API contract also includes macros for defining and installing properties, and for declaring and defining accessor functions - with and without custom code.

the API contract is missing some types, but it should give away the gist of it; I'm also finishing the implementation, but I wanted to get a bug opened for further discussion.

one of the bonus points of the design is that it's based off of the current GParamSpec machinery, thus it is fairly non-intrusive and allows migration without breaking backward compatibility.
Comment 1 Emmanuele Bassi (:ebassi) 2011-04-23 22:00:32 UTC
Created attachment 186535 [details]
example of GProperty usage

a simple example of how GProperty and friends would look like in a new project
Comment 2 Emmanuele Bassi (:ebassi) 2011-04-23 22:01:50 UTC
as a side node: I'm seriously considering using at least the macros for the accessors in this development cycle of Clutter, since we have tons and tons of these kind of simple setter/getter pairs and they make my life really miserable.
Comment 3 Dan Winship 2011-04-24 00:29:44 UTC
Comment on attachment 186534 [details]
initial version of the GProperty API contract

Love it. Now with that out of the way, here's a bunch of complaints. :)

> * G_PROPERTY_WITH_CODE:

Confusing name... G_PROPERTY_RANGE() doesn't feel like "code", even though that's what it expands to.

Would be nice if there was some way to not have to have a separate _WITH_CODE macro... Can you set the range after installing the property? Then you could do:

  G_PROPERTY (ExampleObject, example_object,
              int, foo,
              _("Foo"),
              _("The value of foo"));
  G_PROPERTY_SET_RANGE (ExampleObject, example_object,
                        foo, int,
                        -1, G_MAXINT, 42);

Also, it seems to me that you should not need to specify the type in G_PROPERTY_RANGE / G_PROPERTY_SET_RANGE; you can just have a single g_property_set_range() that uses varargs to read the values. (There are already macros to read a varargs arg of the correct type for a given GValue.)

Also, it's ugly that range uses a macro, but default value (for non-ranged types) doesn't.

Also, it would be nice to not have to specify "ExampleObject, example_object" every time... this could work if G_DEFINE_TYPE did... oh... no, it couldn't. Hm... Well, you could have the author do:

  #define G_TYPE_NAME "ExampleObject"
  #define G_TYPE_PREFIX "example_object"

and then have G_DEFINE* use those automatically (though of course the existing G_DEFINE_TYPE(), etc, wouldn't be able to... although actually, we could have

  #ifdef G_TYPE_NAME
    #define G_DEFINE_TYPE(parent_type) ... new macro ...
  #else
    #define G_DEFINE_TYPE(T_n, t_n, parent_type) ... old macro ...
  #endif

(although having it take just parent_type would be weird, so there should be a G_TYPE_PARENT_TYPE too).)

Anyway:

  G_PROPERTY (int, foo,
              _("Foo"),
              _("The value of foo"));
  G_PROPERTY_SET_RANGE (foo, -1, G_MAXINT, 42);

Though really... the nick and blurb are *completely* useless if the object is not something that would be used in a widget builder. (Yes, they get pulled into the documentation by default, but without spiffy gtk-doc cross linkage, so it's better to do the documentation via comments instead anyway.).

So:

  G_PROPERTY (int, foo);
  G_PROPERTY_SET_BLURB (foo, _("Foo"), _("The value of foo"));
  G_PROPERTY_SET_RANGE (foo, -1, G_MAXINT, 42);

(where only the first is mandatory)

(If it's not possible to modify properties after installing them, it would still be possible to do it this way by having a mandatory G_BEGIN_PROPERTIES()/G_END_PROPERTIES() around it all... Or even G_CLASS_INIT_BEGIN/G_CLASS_INIT_END...)

>/*< private >

I don't think you really meant most of these declarations to be private.

> * G_DEFINE_PROPERTY:

I would call this G_DECLARE_PROPERTY, though I'm not totally sure I like it anyway. It's one thing to automate writing implementations, but it seems wrong to automate writing header files the same way. I don't really have any actual argument for why though.

It's also weird that it declares a setter even if the property is read-only. Though that's mostly harmless I guess. ("Mostly" because it means a library that mistakenly calls a non-existant setter could compile without warnings, and only fail when it was linked into a program.)

> * This macro should be used when intending to notify on #GObject properties,

Notifying should be the default. If people want a setter that doesn't notify, they can write it themselves. Even if you don't need the notification, someone else will want it later, and g_object_notify() isn't all that expensive if no one is listening anyway.

> * @ret: default return value (mostly for error conditions) for the getter
> *   function

You could just return the default value... it's an error condition, so it's difficult to argue that some other specific value would be better. (Yes, sometimes there's a value you can return that's more likely to make the caller stop trying to use the dead object, but, still... Maybe if you fail harder people will fix their broken callers! :)

> * G_DEFINE_PROPERTY_SYNTH:

I would call this "G_DEFINE_PROPERTY_ACCESSORS". Also, it's again weird that this defines a setter even if the property is read-only. I suppose in that case you're supposed to use G_DEFINE_PROPERTY_SYNTH_GET() rather than G_DEFINE_PROPERTY_SYNTH(), but that seems fiddly and prone to error. Though I can't think of any way around it... In that case, maybe the better thing to do would be to call it G_DEFINE_PROPERTY_GET_SET(), so it's totally obvious.
Comment 4 Emmanuele Bassi (:ebassi) 2011-04-24 13:21:50 UTC
(In reply to comment #3)
> (From update of attachment 186534 [details])
> Love it. Now with that out of the way, here's a bunch of complaints. :)
> 
> > * G_PROPERTY_WITH_CODE:
> 
> Confusing name... G_PROPERTY_RANGE() doesn't feel like "code", even though
> that's what it expands to.

it's in the same spirit as G_IMPLEMENT_INTERFACE(), but I'm not overly sold on it.

> Would be nice if there was some way to not have to have a separate _WITH_CODE
> macro... Can you set the range after installing the property? Then you could
> do:
> 
>   G_PROPERTY (ExampleObject, example_object,
>               int, foo,
>               _("Foo"),
>               _("The value of foo"));
>   G_PROPERTY_SET_RANGE (ExampleObject, example_object,
>                         foo, int,
>                         -1, G_MAXINT, 42);

sounds okay to me; though ranges are not always set or useful, and this would require adding new macros for non-range property types.

> Also, it seems to me that you should not need to specify the type in
> G_PROPERTY_RANGE / G_PROPERTY_SET_RANGE; you can just have a single
> g_property_set_range() that uses varargs to read the values. (There are already
> macros to read a varargs arg of the correct type for a given GValue.)

that was another option that I briefly considered before using the macro approach; could definitely be done, but again: not every property has a range.

another option would be annotating the properties with key/value pairs, like:

  g_property_annotate (property, "minValue", -1, "maxValue", G_MAXINT, "defaultValue", 42, NULL);

using the same format for annotations as the introspection XML uses.

> Also, it would be nice to not have to specify "ExampleObject, example_object"
> every time... this could work if G_DEFINE_TYPE did... oh... no, it couldn't.
> Hm... Well, you could have the author do:
> 
>   #define G_TYPE_NAME "ExampleObject"
>   #define G_TYPE_PREFIX "example_object"
> 
> and then have G_DEFINE* use those automatically (though of course the existing
> G_DEFINE_TYPE(), etc, wouldn't be able to... although actually, we could have
> 
>   #ifdef G_TYPE_NAME
>     #define G_DEFINE_TYPE(parent_type) ... new macro ...
>   #else
>     #define G_DEFINE_TYPE(T_n, t_n, parent_type) ... old macro ...
>   #endif
> 
> (although having it take just parent_type would be weird, so there should be a
> G_TYPE_PARENT_TYPE too).)

this would not be at all bad.

> Anyway:
> 
>   G_PROPERTY (int, foo,
>               _("Foo"),
>               _("The value of foo"));
>   G_PROPERTY_SET_RANGE (foo, -1, G_MAXINT, 42);
> 
> Though really... the nick and blurb are *completely* useless if the object is
> not something that would be used in a widget builder. (Yes, they get pulled
> into the documentation by default, but without spiffy gtk-doc cross linkage, so
> it's better to do the documentation via comments instead anyway.).
> 
> So:
> 
>   G_PROPERTY (int, foo);
>   G_PROPERTY_SET_BLURB (foo, _("Foo"), _("The value of foo"));
>   G_PROPERTY_SET_RANGE (foo, -1, G_MAXINT, 42);
> 
> (where only the first is mandatory)
> 
> (If it's not possible to modify properties after installing them, it would
> still be possible to do it this way by having a mandatory
> G_BEGIN_PROPERTIES()/G_END_PROPERTIES() around it all... Or even
> G_CLASS_INIT_BEGIN/G_CLASS_INIT_END...)

sadly, it's still possible to install properties at any point, given the class pointer.
 
> >/*< private >
> 
> I don't think you really meant most of these declarations to be private.

no; it's just copy and paste from the clutter's macro file that I'm working on.

> > * G_DEFINE_PROPERTY:
> 
> I would call this G_DECLARE_PROPERTY, though I'm not totally sure I like it
> anyway. It's one thing to automate writing implementations, but it seems wrong
> to automate writing header files the same way. I don't really have any actual
> argument for why though.

my main argument is ease of parsing for stuff like g-ir-scanner and gtk-doc.

and G_DECLARE_PROPERTY is a better name, I agree.

> It's also weird that it declares a setter even if the property is read-only.
> Though that's mostly harmless I guess. ("Mostly" because it means a library
> that mistakenly calls a non-existant setter could compile without warnings, and
> only fail when it was linked into a program.)

if it's read-only you can definitely just use the SYNTH_GET/SYNTH_SET macros instead.
 
> > * This macro should be used when intending to notify on #GObject properties,
> 
> Notifying should be the default. If people want a setter that doesn't notify,
> they can write it themselves. Even if you don't need the notification, someone
> else will want it later, and g_object_notify() isn't all that expensive if no
> one is listening anyway.

actually, my main point against this is that notifying using the name *is* more expensive; that was the whole point of storing the pspecs inside an array and using g_object_notify_by_pspec(). using that by default, though, would imply some static naming, e.g.:

  enum { PROP_0, ...., LAST_PROP };

  static GParamSpec obj_props[LAST_PROP] = { NULL, }

that the pre-processor cannot cope with.

> > * @ret: default return value (mostly for error conditions) for the getter
> > *   function
> 
> You could just return the default value... it's an error condition, so it's
> difficult to argue that some other specific value would be better. (Yes,
> sometimes there's a value you can return that's more likely to make the caller
> stop trying to use the dead object, but, still... Maybe if you fail harder
> people will fix their broken callers! :)

this is a valid point, and I like it much better.

> > * G_DEFINE_PROPERTY_SYNTH:
> 
> I would call this "G_DEFINE_PROPERTY_ACCESSORS". Also, it's again weird that
> this defines a setter even if the property is read-only. I suppose in that case
> you're supposed to use G_DEFINE_PROPERTY_SYNTH_GET() rather than
> G_DEFINE_PROPERTY_SYNTH(), but that seems fiddly and prone to error. Though I
> can't think of any way around it... In that case, maybe the better thing to do
> would be to call it G_DEFINE_PROPERTY_GET_SET(), so it's totally obvious.

I used "synth" because of the @synthesize pragma in obj-c 2.0, but again: I'm not overly sold on naming anyway, and DEFINE_PROPERTY_ACCESSORS looks fine to me.
Comment 5 Dan Winship 2011-04-24 18:53:34 UTC
(In reply to comment #4)
> >   G_PROPERTY (ExampleObject, example_object,
> >               int, foo,
> >               _("Foo"),
> >               _("The value of foo"));
> >   G_PROPERTY_SET_RANGE (ExampleObject, example_object,
> >                         foo, int,
> >                         -1, G_MAXINT, 42);
> 
> sounds okay to me; though ranges are not always set or useful, and this would
> require adding new macros for non-range property types.

Huh? Non-range property types would just use G_PROPERTY(), and not call G_PROPERTY_SET_RANGE() after it. (Well, I guess you'd need G_PROPERTY_SET_DEFAULT() too, but you already have that, at least as a non-macro.)

> > Also, it seems to me that you should not need to specify the type in
> > G_PROPERTY_RANGE / G_PROPERTY_SET_RANGE; you can just have a single
> > g_property_set_range() that uses varargs to read the values. (There are already
> > macros to read a varargs arg of the correct type for a given GValue.)
> 
> that was another option that I briefly considered before using the macro
> approach; could definitely be done, but again: not every property has a range.

More confusion. I just meant that instead of having 14 simple

    void g_TYPE_property_set_range (GProperty *, TYPE, TYPE, TYPE);

methods, you'd have one complicated

    void g_property_set_range (GProperty *, ...);

method, that used the type information that's already in the GProperty to figure out how to va_arg out the remaining arguments and put them in the right place in the GProperty. Then G_PROPERTY_RANGE (or G_PROPERTY_SET_RANGE) no longer needs to take the type as one of its arguments.

> > I would call this G_DECLARE_PROPERTY, though I'm not totally sure I like it
> > anyway. It's one thing to automate writing implementations, but it seems wrong
> > to automate writing header files the same way. I don't really have any actual
> > argument for why though.
> 
> my main argument is ease of parsing for stuff like g-ir-scanner and gtk-doc.

They don't really have much difficulty parsing simple getter and setter declarations though.

> if it's read-only you can definitely just use the SYNTH_GET/SYNTH_SET macros
> instead.

right, there's a macro to *implement* only a getter, but no (public) macro to *declare* only a getter.

> actually, my main point against this is that notifying using the name *is* more
> expensive; that was the whole point of storing the pspecs inside an array and
> using g_object_notify_by_pspec(). using that by default, though, would imply
> some static naming, e.g.:
> 
>   enum { PROP_0, ...., LAST_PROP };
> 
>   static GParamSpec obj_props[LAST_PROP] = { NULL, }
> 
> that the pre-processor cannot cope with.

Hm... we could steal one of the remaining padding slots in GObjectClass?...

So many things we could do if macros could define macros...
Comment 6 Emmanuele Bassi (:ebassi) 2011-04-24 23:01:54 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > >   G_PROPERTY (ExampleObject, example_object,
> > >               int, foo,
> > >               _("Foo"),
> > >               _("The value of foo"));
> > >   G_PROPERTY_SET_RANGE (ExampleObject, example_object,
> > >                         foo, int,
> > >                         -1, G_MAXINT, 42);
> > 
> > sounds okay to me; though ranges are not always set or useful, and this would
> > require adding new macros for non-range property types.
> 
> Huh? Non-range property types would just use G_PROPERTY(), and not call
> G_PROPERTY_SET_RANGE() after it. (Well, I guess you'd need
> G_PROPERTY_SET_DEFAULT() too, but you already have that, at least as a
> non-macro.)

that was my point exactly. :-)

I don't particularly like the repetition of the field type/name, though, because it makes it more prone to typos; it would be nice to have a BEGIN_PROP/END_PROP, but then it would look like crap with lots of properties - take ClutterActor, for instance.

so, we can have a single G_PROPERTY macro with a last optional argument, e.g.:

  #define G_PROPERTY (field_type, field_name, flags, ...)

used as:

  G_PROPERTY (int, foo, G_PROPERTY_READWRITE, G_PROPERTY_SET_RANGE (-1, G_MAXINT, 42));
  G_PROPERTY (float, bar, G_PROPERTY_READWRITE);
  G_PROPERTY (string, baz, G_PROPERTY_READWRITE, G_PROPERTY_DEFAULT ("blah"));

> > > Also, it seems to me that you should not need to specify the type in
> > > G_PROPERTY_RANGE / G_PROPERTY_SET_RANGE; you can just have a single
> > > g_property_set_range() that uses varargs to read the values. (There are already
> > > macros to read a varargs arg of the correct type for a given GValue.)
> > 
> > that was another option that I briefly considered before using the macro
> > approach; could definitely be done, but again: not every property has a range.
> 
> More confusion. I just meant that instead of having 14 simple
> 
>     void g_TYPE_property_set_range (GProperty *, TYPE, TYPE, TYPE);
> 
> methods, you'd have one complicated
> 
>     void g_property_set_range (GProperty *, ...);
> 
> method, that used the type information that's already in the GProperty to
> figure out how to va_arg out the remaining arguments and put them in the right
> place in the GProperty.

this was what I understood. :-)

my point was that it would have to warn when used on a property type that does not support a range; but then you lose some type safety.

> > > I would call this G_DECLARE_PROPERTY, though I'm not totally sure I like it
> > > anyway. It's one thing to automate writing implementations, but it seems wrong
> > > to automate writing header files the same way. I don't really have any actual
> > > argument for why though.
> > 
> > my main argument is ease of parsing for stuff like g-ir-scanner and gtk-doc.
> 
> They don't really have much difficulty parsing simple getter and setter
> declarations though.

many possible styles that require parsing C or ridiculous regular expressions vs. a single, well defined style.

> > if it's read-only you can definitely just use the SYNTH_GET/SYNTH_SET macros
> > instead.
> 
> right, there's a macro to *implement* only a getter, but no (public) macro to
> *declare* only a getter.

yes, that was my fault: my plan was to have:

  G_DEFINE_PROPERTY_SET
  G_DEFINE_PROPERTY_GET

and a

  G_DEFINE_PROPERTY

combining the two - but then I skipped the former two in the proposal I attached.

> > actually, my main point against this is that notifying using the name *is* more
> > expensive; that was the whole point of storing the pspecs inside an array and
> > using g_object_notify_by_pspec(). using that by default, though, would imply
> > some static naming, e.g.:
> > 
> >   enum { PROP_0, ...., LAST_PROP };
> > 
> >   static GParamSpec obj_props[LAST_PROP] = { NULL, }
> > 
> > that the pre-processor cannot cope with.
> 
> Hm... we could steal one of the remaining padding slots in GObjectClass?...

in theory, yes: I'd attach the pspec array to the class structure (instead of using the ParamSpecPool, which is another performance failure); I'd need a PtrArray, though, to append the GProperty pointers. the enumeration would still need to be defined by the developer, though, because the list of properties is not available programmatically.

we could specify the GParamSpec to notify with - and if NULL it would use the field name and g_object_notify() instead.

in short, to create a GObject class in the Brave New World(tm), you'd need:

#define G_TYPE_NAME ExampleObject
#define G_TYPE_PREFIX example_object
#define G_TYPE_PARENT_TYPE G_TYPE_OBJECT

G_DEFINE_TYPE();

struct _ExampleObjectPrivate
{
  int foo;
  gchar *bar;
};

enum { PROP_0, PROP_FOO, PROP_BAR, PROP_LAST };

static GParamSpec *obj_props[PROP_LAST] = { NULL, };

static void example_object_class_init (ExampleObjectClass *klass)
{
  g_type_class_add_private (klass, sizeof (ExampleObjectPrivate));

  G_PROPERTY (int, foo, G_PROPERTY_READWRITE, G_PROPERTY_RANGE (-1, G_MAXINT, 42));
  G_PROPERTY (string, bar, G_PROPERTY_READABLE);
}

static void example_object_init (ExampleObject *self)
{
  self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, EXAMPLE_TYPE_OBJECT, ExampleObjectPrivate);
  self->priv->foo = 42;
}

G_DECLARE_PROPERTY_GET_SET (int, foo, obj_props[PROP_FOO]);
G_DECLARE_PROPERTY_GET (string, bar, obj_props[PROP_BAR]);

> So many things we could do if macros could define macros...

hehe
Comment 7 Colin Walters 2011-04-25 22:00:41 UTC
(In reply to comment #3)
> 
> Though really... the nick and blurb are *completely* useless if the object is
> not something that would be used in a widget builder. (Yes, they get pulled
> into the documentation by default, but without spiffy gtk-doc cross linkage, so
> it's better to do the documentation via comments instead anyway.).

So the current proposal will generate undocumented C entry points.  I guess gtk-doc would have to know about these macros.

Also...I suggest we default the nick to uppercasing the property name, and default the blurb to "".  GStreamer apparently uses the blurb for "gst-inspect"; we could give them a G_DECLARE_PROPERTY_FULL() which allows passing it.

Glade should just be rendering the gtk-doc IMO.
Comment 8 Emmanuele Bassi (:ebassi) 2011-04-25 23:14:08 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > 
> > Though really... the nick and blurb are *completely* useless if the object is
> > not something that would be used in a widget builder. (Yes, they get pulled
> > into the documentation by default, but without spiffy gtk-doc cross linkage, so
> > it's better to do the documentation via comments instead anyway.).
> 
> So the current proposal will generate undocumented C entry points.  I guess
> gtk-doc would have to know about these macros.

AFAIR, the gtk-doc scanner will just gather a list of properties and pre-fill the documentation slots using the introspection API from GObjectClass; then it'll parse the documentation blurb to fill out the gap. that's how it also gathers the default value and the min/max allowed range.

making gtk-doc parse the actual macros would at least remove the need to use the scanner to generate the list of properties of a class (though it'll still need it to parse the signals, until we remove that as well).
 
> Also...I suggest we default the nick to uppercasing the property name, and
> default the blurb to "".  GStreamer apparently uses the blurb for
> "gst-inspect"; we could give them a G_DECLARE_PROPERTY_FULL() which allows
> passing it.

I also have the Evil Plan(tm) of allowing custom annotations, for third-party extension - like Glade, GStreamer and friends, and moving the nick/blurb/range/default value and other currently fixed data to the annotation system.
Comment 9 Emmanuele Bassi (:ebassi) 2011-04-30 23:59:26 UTC
Created attachment 186952 [details]
GProperty API contract / v2.0

new version of the API, after Dan's comments. I have a rough implementation for the API, but it'll need some more testing before I can actually submit a patch.
Comment 10 Benjamin Otte (Company) 2011-05-01 00:36:46 UTC
I'm not sure I like the macro madness, in particular it makes the code very hard to digest for newcomers that want to read and understand it. They will usually git grep for the function and not find it. That said, If we keep the defines next to the doc comments, grepping for it should work, so I'm only semi-uneasy about it.

Also, the code for setters does not do sanity checks. So if we declare an int range property, we want the setter to return_if_fail() for >= min and <= max.

Then the macros don't do the strdup and ref/unref that we need for strings and objects or the memcpy() we do for boxed types.

I do not like the DEFINE_WITH_CODE() versions because they expose variable names (self, cur_value).

I don't think we win a lot with the G_PROPERTY() macro and all its friends. We lose readability and ease of understanding though. I mean
g_object_class_install_property (g_property_new(..)) is only 4 lines. And everyone can follow it, even if he's not knowing gobject/*.h by heart.

I'm not sure if you meant for GProperty to be a subclass of GParamSpec. If so, why do we have GPropertyFlags and g_property_get/set_foo() and don't add those things to the GParamSpec API? If not, why not?
Comment 11 Benjamin Otte (Company) 2011-05-01 00:44:25 UTC
Hrm, looking at the setter madness, what about having something like this:

int g_property_int_get_value (GProperty *property, gpointer instance);
void g_property_int_set_value (GProperty *property, gpointer instance, int value);

which would call the vfuncs provided in the property definition or by default do what the macro does now.

Then you could have a properties[LAST_PROPERTY] array in the .c file like you have a signals[LAST_SIGNAL] array today.

And then a default setter would look like this:
void
clutter_actor_set_margin_top (ClutterActor *actor, int margin_top)
{
  g_property_int_set_value (properties[MARGIN_TOP], actor, margin_top);
}

And all of that without macro madness.
Comment 12 Matthias Clasen 2011-05-04 11:51:48 UTC
(In reply to comment #10)
> I'm not sure I like the macro madness, in particular it makes the code very
> hard to digest for newcomers that want to read and understand it. 

[...]
 
> I don't think we win a lot with the G_PROPERTY() macro and all its friends. We
> lose readability and ease of understanding though.

Thats my view, too.
Comment 13 Emmanuele Bassi (:ebassi) 2011-05-05 16:54:58 UTC
Created attachment 187297 [details] [review]
gobject: Add GProperty / v0.1

this is a preliminary patch against current master; it is incomplete, as it is missing the generic setter/getter/validator for all types except gboolean and gint, but it works as a proof of concept.

the ultimate goal is, obviously, to avoid GValue wrappers whenever is possible. the typesafe accessors can be made public, but right now I didn't want to deal with lots of new symbols.
Comment 14 Matthias Clasen 2011-05-05 20:34:53 UTC
Review of attachment 187297 [details] [review]:

Not really convinced about the atomic stuff - a hash table of mutexes, one per property ?
Don't we want to at least optionally make use of object locks for that ? or some facility to bind objects to threads ?

::: gobject/gproperty.h
@@ +519,3 @@
+ * |[
+ *   G_DEFINE_PROPERTY_ACCESSORS (ClutterActor, clutter_actor,
+ *                                int, margin_top)

copy-paste ? s/ACCESSORS/GET_SET/ ?
Comment 15 Emmanuele Bassi (:ebassi) 2011-05-05 21:05:42 UTC
(In reply to comment #14)
> Review of attachment 187297 [details] [review]:
> 
> Not really convinced about the atomic stuff - a hash table of mutexes, one per
> property ?

it was mostly an idea, and yes: the implementation is suboptimal (if not entirely wrong :-)).

> Don't we want to at least optionally make use of object locks for that ? or
> some facility to bind objects to threads ?

that was my plan; I wanted to get feedback on the possibility of automating the atomicity of property setting without having to defer to the application developer.

> ::: gobject/gproperty.h
> @@ +519,3 @@
> + * |[
> + *   G_DEFINE_PROPERTY_ACCESSORS (ClutterActor, clutter_actor,
> + *                                int, margin_top)
> 
> copy-paste ? s/ACCESSORS/GET_SET/ ?

yep; the whole documentation is just a work in progress.
Comment 16 Emmanuele Bassi (:ebassi) 2011-05-10 17:41:27 UTC
Created attachment 187585 [details] [review]
gobject: Add GProperty / v0.2

okay, this is version 0.2. it incorporates many fixes to the API and implementation suggested by Company, but it's still a little bit too messy to actually propose it for merging.

what's missing:

  - examples, lots of;
  - more documentation;
  - more tests;
  - interface support;
  - cleaner overriding of default values for sub-classes;
  - hand-written implementation of properties for a bunch of sub-types,
    like enum/flags and float/double
  - a GVariant property

what it does have, though, is the ability to go through:

    g_object_set (self, "foo", 42, NULL)
  → g_property_set (prop_foo, self, 42)
  → self->priv->foo = 42

without touching GValue. I'll need a proper performance benchmark to verify the gain in terms of cycles, but on paper we should shave off between 15% and 25% by simply not having to allocate/free GValue contents in a tight loop (think animating a property at 60 FPS).
Comment 17 Emmanuele Bassi (:ebassi) 2011-05-10 17:44:51 UTC
oh, was forgetting: it's also missing a default (object, property) locking implementation
Comment 18 Benjamin Otte (Company) 2011-05-10 17:47:56 UTC
(In reply to comment #17)
> oh, was forgetting: it's also missing a default (object, property) locking
> implementation

I guess this can be added later, as long as we reserve extra property flags?
The reason I'm asking is that I'd like to keep the initial merge small, otherwise we'll get stuck bikeshedding over details for too long and nothing ever happens...
Comment 19 Emmanuele Bassi (:ebassi) 2011-05-10 19:41:27 UTC
(In reply to comment #18)
> (In reply to comment #17)
> > oh, was forgetting: it's also missing a default (object, property) locking
> > implementation
> 
> I guess this can be added later, as long as we reserve extra property flags?
> The reason I'm asking is that I'd like to keep the initial merge small,
> otherwise we'll get stuck bikeshedding over details for too long and nothing
> ever happens...

sure, it's not a blocking thing.

the API is already there, though, and it's just missing a nicer implementation that the one I used in v0.1; ideally, some way to just lock the (object, property) tuple, which could also be interesting as a side-project. in any case, it's possible to override the locking functions when creating a property, so that projects like GStreamer can do their thing.
Comment 20 Emmanuele Bassi (:ebassi) 2011-05-16 14:36:39 UTC
Created attachment 187918 [details] [review]
gobject: Add GProperty / v0.3

this is the current state of GProperty, to a point that I feel comfortable enough for review.

I added the long description, with example code both inlined and xincluded from full examples built along with gobject.

flags, enums, floats and doubles are handled using handwritten rules instead of macros.

the default atomic locking is implemented using a uniquely named bit lock attached to the object.

default overriding is still a bit in flux, and I'd like feedback on that - the documentation mentions g_property_override_default(), but after playing with the API, I still think that a single function taking the class pointer is the simplest approach; g_object_class_override_property_default() can be added to make it even simpler.

interface support: this is a bit of a sore point; properties on interface can be installed by simply passing function pointers instead of the offset, but the whole machinery in GObject/GInterface relies on GParamSpecOverride to redirect to the correct GParamSpec. this would mean that:

  - the interface installs a GProperty with the setter/getter functions
  - the implementation calls g_object_class_override_property() which creates a GParamSpecOverride
  - when calling g_object_set/get on the instance, the override is retrieved and the setter/getter attached to the GProperty are invoked (this is currently missing from the patch)
  - the interface implementation has to do something like calling virtual functions
  - the implementation implements the virtual functions

so we have a bunch of indirections: gobject -> gproperty -> interface -> instance.

one possible approach is being able to override the setter/getter functions when overriding the property for an interface implementation, so that an interface can install a property with no setter/getter/field offset, and an implementation would call:

  g_object_class_override_property_accessors (klass, "foo",
                                              my_class_set_foo,
                                              my_class_get_foo);

this would only work for properties with no accessors already set; when trying to set/get a property with no accessors/field offset, we already warn, so it wouldn't require additional checks.
Comment 21 Benjamin Otte (Company) 2011-05-16 15:49:52 UTC
(In reply to comment #20)
> interface support: this is a bit of a sore point; properties on interface can
> be installed by simply passing function pointers instead of the offset, but the
> whole machinery in GObject/GInterface relies on GParamSpecOverride to redirect
> to the correct GParamSpec. this would mean that:
> 
>   - the interface installs a GProperty with the setter/getter functions
>   - the implementation calls g_object_class_override_property() which creates a
> GParamSpecOverride
>   - when calling g_object_set/get on the instance, the override is retrieved
> and the setter/getter attached to the GProperty are invoked (this is currently
> missing from the patch)
>   - the interface implementation has to do something like calling virtual
> functions
>   - the implementation implements the virtual functions
> 
> so we have a bunch of indirections: gobject -> gproperty -> interface ->
> instance.
> 
First of all, I would avoid the override. If we have a GProperty on the interface, that is already enough to do all the work of getting and setting.
Second, is this really that bad? I mean, you go
  gobject -> gproperty -> instance
anyway if you use g_object_set(), so one more vfunc isn't that bad, in particular because we know that interfaces are slow.
Second, you'd use the setters in normal code and those can now call the vfunc directly instead of going via g_object_set() (ugh: http://git.gnome.org/browse/gtk+/tree/gtk/gtkfilechooser.c#n1881 )
Comment 22 Matthias Clasen 2011-05-17 04:02:17 UTC
Review of attachment 187918 [details] [review]:

::: gobject/gproperty.c
@@ +30,3 @@
+ * The main difference between #GProperty and #GParamSpec is that #GProperty
+ * enforces a specific set of best practices for accessing values exposed
+ * and #GObject properties.

s/and/as/ ?

@@ +172,3 @@
+ *
+ *     <para>Validation is automatically performed when setting a property
+ *     through g_object_set(); explicit setter methods can use g_property_validate()

s/g_object_set/g_property_set/ here ?

@@ +204,3 @@
+ *   <refsect3>
+ *     <title>Default values</title>
+ *     <para>Default values are set using g_property_set_default(), e.g.:</para>

s/set/declared/ ? you still have to set the default value in _init(), right ?
Comment 23 Matthias Clasen 2011-05-17 04:03:10 UTC
Review of attachment 187918 [details] [review]:

Would be good to have a performance test to substantiate the claim of being faster than GValue-based properties.
Comment 24 Emmanuele Bassi (:ebassi) 2011-05-17 13:01:36 UTC
Created attachment 187957 [details] [review]
gobject: Add GProperty / v0.4

small fixes over the documentation, after Matthias review.

plus less smaller fixes over the implementation, where I identified a bunch of redundant type checks and some stupid stuff in the locking/unlocking.
Comment 25 Emmanuele Bassi (:ebassi) 2011-05-17 13:03:14 UTC
Created attachment 187958 [details] [review]
perf: Compare GParamSpec and GProperty usage

Use g_object_set() and g_object_get(), since direct function access is
not an interesting benchmark.

The code simply sets and gets the properties of a GObject class, once
created using GParamSpec and overriding GObject::set_property and
GObject::get_property virtual functions; then using GProperty and direct
access over the private data structure field. The implementation of the
GParamSpec accessors are equivalent to the GProperty internal code.

On my X200s, with a 1.86 GHz Core 2 Duo, the results are:

  Running test gparamspec-property
  Number of accesses per second: 103422
  Running test gproperty-property
  Number of accesses per second: 155550

for 3 properties of types int, gchar* and float respectively, and

  Running test gparamspec-property
  Number of accesses per second: 123298
  Running test gproperty-property
  Number of accesses per second: 259618

for 2 properties of types int and float.
Comment 26 Emmanuele Bassi (:ebassi) 2011-05-17 16:28:44 UTC
Created attachment 187972 [details] [review]
gobject: Add override_property_default()

For object properties defined using the GProperty API we can easily
override the default value from sub-classes; GObject should have a
convenience method that does all the automagic heavy lifting so that the
developer can just provide the property name and the new default value
inside the class initialization function.
Comment 27 Matthias Clasen 2011-05-18 01:46:58 UTC
Review of attachment 187957 [details] [review]:

::: gobject/gproperty.c
@@ +444,3 @@
+  gssize type_size;
+  gssize priv_offset;
+  gssize field_offset;

Considering both class size and private size are stored as guint16 in gtype, these fields can probably be a bit smaller

@@ +453,3 @@
+  GPropertyUnlockFunc unlock_func;
+
+  guint is_installed : 1;

Might be nice to fold that bit into the flags further up

@@ +3450,3 @@
+
+  if (property->default_values == NULL)
+    property->default_values = g_hash_table_new_full (g_str_hash, g_str_equal,

Type names are internalized, so we can use g_direct_hash/g_direct_equal here and don't need to duplicate the keys, I think

@@ +3453,3 @@
+                                                      g_free,
+                                                      g_free);
+

Also, this means we always create a hash table per property if we set a default value for it at all ? Last time I looked, GHashTable came in at ~96 bytes, so that is quite a bit of overhead that would be nice to avoid for the common case of non-overridden default values.

@@ +5180,3 @@
+  gint *bit_lock_p;
+
+  bit_lock_p = g_object_get_data (gobject, lock_name);

It is a little dubious to use object data for storing per-object locks, since these get/set_data calls take a global lock anyway.

Also, g_object_get_data calls g_quark_try_string, which is yet another global lock. Should use qdata here, at least.

Finally, constructing the lock name for every lock or unlock call is bad news, I'd say
Comment 28 Emmanuele Bassi (:ebassi) 2011-05-18 06:51:55 UTC
(In reply to comment #27)
> Review of attachment 187957 [details] [review]:
> 
> ::: gobject/gproperty.c
> @@ +444,3 @@
> +  gssize type_size;
> +  gssize priv_offset;
> +  gssize field_offset;
> 
> Considering both class size and private size are stored as guint16 in gtype,
> these fields can probably be a bit smaller

fixed.
 
> @@ +453,3 @@
> +  GPropertyUnlockFunc unlock_func;
> +
> +  guint is_installed : 1;
> 
> Might be nice to fold that bit into the flags further up

using private flags or having a G_PROPERTY_IS_INSTALLED public flag (and accessor)? the flag should probably be filtered out (with a warning) when creating the GProperty, to avoid people passing it and screwing up the internal state.

> @@ +3450,3 @@
> +
> +  if (property->default_values == NULL)
> +    property->default_values = g_hash_table_new_full (g_str_hash, g_str_equal,
> 
> Type names are internalized, so we can use g_direct_hash/g_direct_equal here
> and don't need to duplicate the keys, I think

fixed.

> @@ +3453,3 @@
> +                                                      g_free,
> +                                                      g_free);
> +
> 
> Also, this means we always create a hash table per property if we set a default
> value for it at all ? Last time I looked, GHashTable came in at ~96 bytes, so
> that is quite a bit of overhead that would be nice to avoid for the common case
> of non-overridden default values.

fixed; it now uses a single GValue* when calling g_property_set_default() the first time.

> @@ +5180,3 @@
> +  gint *bit_lock_p;
> +
> +  bit_lock_p = g_object_get_data (gobject, lock_name);
> 
> It is a little dubious to use object data for storing per-object locks, since
> these get/set_data calls take a global lock anyway.

yes. I was planning something inside GObject, something like g_object_lock()/unlock() using a bitlock on the GData's flags, but I think that requires a bit more discussion and a separate bug — especially if it's going to be public API. this implementation is basically a lo-fi version of that plan.

> Also, g_object_get_data calls g_quark_try_string, which is yet another global
> lock. Should use qdata here, at least.

> Finally, constructing the lock name for every lock or unlock call is bad news,
> I'd say

both fixed.
Comment 29 Emmanuele Bassi (:ebassi) 2011-05-18 08:17:30 UTC
(In reply to comment #28)
> > +  bit_lock_p = g_object_get_data (gobject, lock_name);
> > 
> > It is a little dubious to use object data for storing per-object locks, since
> > these get/set_data calls take a global lock anyway.
> 
> yes. I was planning something inside GObject, something like
> g_object_lock()/unlock() using a bitlock on the GData's flags

mmh, scratch that, we cannot use the GData flags because the available bits are already gone (0x1 for the toggle-ref flag, 0x2 for the floating flag, 0x3 is the mask).
Comment 30 Alexander Larsson 2011-05-18 09:48:16 UTC
g_object_lock is bug 608695.

Also, for space, remember that we have a 32bit hole in GObject on 64bit arches:

http://blogs.gnome.org/alexl/2009/10/12/gobject-performance-on-64bit-arches/
Comment 31 Matthias Clasen 2011-05-21 02:45:12 UTC
For the g_object_set/get_data locking and performance, see bug 650458
Comment 32 Matthias Clasen 2011-05-21 03:19:06 UTC
(In reply to comment #28)


> > 
> > Might be nice to fold that bit into the flags further up
> 
> using private flags or having a G_PROPERTY_IS_INSTALLED public flag (and
> accessor)? the flag should probably be filtered out (with a warning) when
> creating the GProperty, to avoid people passing it and screwing up the internal
> state.


I just meant rearranging the struct a bit to avoid holes. If you change things to:

struct _GProperty
{
  GParamSpec parent_instance;

  GPropertyFlags flags;
  guint is_installed : 1;

  guint16 type_size;
  guint16 priv_offset;
  guint16 field_offset;

  /* used by the default locking */
  GQuark lock_quark;

  GPropertyLockFunc lock_func;
  GPropertyUnlockFunc unlock_func;

  GType gtype;
  GType class_gtype;
  GType prerequisite;

  GValue *default_value;

  GHashTable *overridden_defaults;
};

pahole says that the struct size goes from 152 to 144 bytes. You can improve further on it by turning is_installed into a flags member, or by storing the flags as guint flags :16. You won't be able to get it down to 2 cachelines unless you do something more radical, like moving the defaults into the GParamSpec GData...
Comment 33 Emmanuele Bassi (:ebassi) 2011-05-21 08:07:58 UTC
I only have a 32bits arch; with a bit of juggling I can get this:

struct _GProperty {
        GParamSpec                 parent_instance;      /*     0    40 */
        guint                      flags:15;             /*    40:17  4 */
        guint                      is_installed:1;       /*    40:16  4 */

        /* Bitfield combined with next fields */

        guint16                    type_size;            /*    42     2 */
        guint16                    priv_offset;          /*    44     2 */
        guint16                    field_offset;         /*    46     2 */
        GType                      gtype;                /*    48     4 */
        GType                      class_gtype;          /*    52     4 */
        GType                      prerequisite;         /*    56     4 */
        GValue *                   default_value;        /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        GQuark                     lock_quark;           /*    64     4 */
        GPropertyLockFunc          lock_func;            /*    68     4 */
        GPropertyUnlockFunc        unlock_func;          /*    72     4 */
        GHashTable *               overridden_defaults;  /*    76     4 */

        /* size: 80, cachelines: 2, members: 14 */
        /* last cacheline: 16 bytes */
};

which would place the commonly accessed fields in the first cacheline.

placing the overridden defaults in the GData would be possible, using g_type_qname() as the key instead of the interned type name. this would leave the locking in the second cacheline (on 32bit). we could even pack the first default value inside the GData and drop the GValue*.
Comment 34 Emmanuele Bassi (:ebassi) 2011-05-21 08:56:06 UTC
Created attachment 188276 [details] [review]
gproperty: Do not box values in g_property_get_range()

The variadic arguments function can just use the va_list API and avoid a
useless boxing/lcopy/unboxing.
Comment 35 Emmanuele Bassi (:ebassi) 2011-05-21 08:56:11 UTC
Created attachment 188277 [details] [review]
property: Pack the GProperty structure

Using pahole to check for structure packing, size and holes we can make
GProperty go (on 32bit archs) from this:

struct _GProperty {
        GParamSpec                 parent_instance;      /*     0    40 */
        GPropertyFlags             flags;                /*    40     4 */
        GType                      gtype;                /*    44     4 */
        GType                      class_gtype;          /*    48     4 */
        guint16                    type_size;            /*    52     2 */
        guint16                    priv_offset;          /*    54     2 */
        guint16                    field_offset;         /*    56     2 */

        /* XXX 2 bytes hole, try to pack */

        GValue *                   default_value;        /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        GHashTable *               overridden_defaults;  /*    64     4 */
        GType                      prerequisite;         /*    68     4 */
        GPropertyLockFunc          lock_func;            /*    72     4 */
        GPropertyUnlockFunc        unlock_func;          /*    76     4 */
        GQuark                     lock_quark;           /*    80     4 */
        guint                      is_installed:1;       /*    84:31  4 */

        /* size: 88, cachelines: 2, members: 14 */
        /* sum members: 86, holes: 1, sum holes: 2 */
        /* bit_padding: 31 bits */
        /* last cacheline: 24 bytes */
};

to this:

struct _GProperty {
        GParamSpec                 parent_instance;      /*     0    40 */
        guint                      flags:15;             /*    40:17  4 */
        guint                      is_installed:1;       /*    40:16  4 */

        /* Bitfield combined with next fields */

        guint16                    type_size;            /*    42     2 */
        guint16                    priv_offset;          /*    44     2 */
        guint16                    field_offset;         /*    46     2 */
        GType                      gtype;                /*    48     4 */
        GType                      prerequisite;         /*    52     4 */
        GValue *                   default_value;        /*    56     4 */
        GQuark                     lock_quark;           /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        GPropertyLockFunc          lock_func;            /*    64     4 */
        GPropertyUnlockFunc        unlock_func;          /*    68     4 */

        /* size: 72, cachelines: 2, members: 12 */
        /* last cacheline: 8 bytes */
};

The flags have been packed with the is_installed bit.

The class_gtype field has been removed; it was used to retrieve the
default of a GProperty through the GParamSpec API for backward
compatibility, but we can do that by using a guard value and some common
code.

The overridden_defaults hash table has been removed by using the GData
inside GParamSpec, using the GType's quark instead of its name as the
key.

There's still room for improvement by moving the default value to the
GData as well, using a generalized lock_quark as its key; that would
allow us to remove the default_value field and save another pointer.
Comment 36 Benjamin Otte (Company) 2011-05-21 11:47:07 UTC
Can we please not shrink flags and enums into n-bit uints?
gcc does not warn when we accidentally add G_PROPERTY_IS_AWESOME = 1 << 15 and strange things will happen. Also, by using guint instead of the flags, we lose compile-time type checking (gcc will no longer complaining about if (property->flags & G_PARAM_SOMETHING)).

Also, do we have checks in place that ensure the guint16 values don't overflow? Why don't we make them gsize as they should be? I don't think it's too uncommon to have types that exceed 65535 bytes. You just have to include some array in the class, and boom you're there.

I'd probably be a bit more sympathetic if this was a frequently used structure and I would expect megabytes of savings, but even if we allocate 1000 of these per app and we'd save 100 bytes here, that'd still just be 100k.
Comment 37 Emmanuele Bassi (:ebassi) 2011-05-21 13:02:40 UTC
(In reply to comment #36)
> Can we please not shrink flags and enums into n-bit uints?
> gcc does not warn when we accidentally add G_PROPERTY_IS_AWESOME = 1 << 15 and
> strange things will happen. Also, by using guint instead of the flags, we lose
> compile-time type checking (gcc will no longer complaining about if
> (property->flags & G_PARAM_SOMETHING)).

given that the GProperty structure is private, I'd expect that adding new flags would by necessity mean that the person adding the flag would also need to check the size of the flag.

> Also, do we have checks in place that ensure the guint16 values don't overflow?

I foresee other issues if we have a 64k offset inside a structure - but yes, checking for overflowing would be good.

> Why don't we make them gsize as they should be? I don't think it's too uncommon
> to have types that exceed 65535 bytes. You just have to include some array in
> the class, and boom you're there.

not entirely sure it's common.

> I'd probably be a bit more sympathetic if this was a frequently used structure
> and I would expect megabytes of savings, but even if we allocate 1000 of these
> per app and we'd save 100 bytes here, that'd still just be 100k.

it's not about saving memory: it's about trying to make the most-accessed fields of the structure fit inside a cacheline, given that it's going to be accessed often enough.

anyway, the patches I attached are optional, and they can be applied at any point.
Comment 38 Matthias Clasen 2011-05-23 01:52:05 UTC
(In reply to comment #36)
> Can we please not shrink flags and enums into n-bit uints?
> gcc does not warn when we accidentally add G_PROPERTY_IS_AWESOME = 1 << 15 and
> strange things will happen. Also, by using guint instead of the flags, we lose
> compile-time type checking (gcc will no longer complaining about if
> (property->flags & G_PARAM_SOMETHING)).

As long as we have type-safe accessors for the flags, I don't think this is a problem. We can easily handle this inside gproperty.c
 
> Also, do we have checks in place that ensure the guint16 values don't overflow?
> Why don't we make them gsize as they should be? I don't think it's too uncommon
> to have types that exceed 65535 bytes. You just have to include some array in
> the class, and boom you're there.

As I said earlier, GType itself stores these sizes in 16bit ints, so reserving more space here for them is entirely pointless.
 
> I'd probably be a bit more sympathetic if this was a frequently used structure
> and I would expect megabytes of savings, but even if we allocate 1000 of these
> per app and we'd save 100 bytes here, that'd still just be 100k.

Thank you very much, but I am not going to make every app use a 100k more just because G_DEFINE_PROPERTY() looks nicer than g_object_class_install_property().
Comment 39 Emmanuele Bassi (:ebassi) 2011-09-25 17:51:03 UTC
as people keep subscribing: the patches attached to this bug are obsolete. the current state of gproperty can be tracked inside the g-property branch of glib:

  http://git.gnome.org/browse/glib/log/?h=g-property
Comment 40 Javier Jardón (IRC: jjardon) 2012-08-06 17:27:35 UTC
The code lives now in http://git.gnome.org/browse/glib/log/?h=wip/gproperty
Comment 41 Stefan Sauer (gstreamer, gtkdoc dev) 2012-08-12 08:29:05 UTC
In http://git.gnome.org/browse/glib/tree/tests/gobject/performance.c?h=wip/gproperty#n664

one will call this for each proprty:
g_property_get_default ((GProperty *) property_props[PROP_PROPERTY_FOO],
                          self,
                          &(self->priv->foo));

would it make sense to have a convenience method for this? Something like:
g_object_init_properties (self);
Comment 42 Stefan Sauer (gstreamer, gtkdoc dev) 2012-08-12 08:55:05 UTC
http://git.gnome.org/browse/glib/tree/gobject/gproperty.c?h=wip/gproperty#n121
typo: s/there are not #GObject set_property/there are no #GObject set_property/

http://git.gnome.org/browse/glib/tree/gobject/gproperty.c?h=wip/gproperty#n182
wouldn't it be sufficient to have one property in the g_property_set_range() and g_property_set_default() examples. Also it looks wrong to me to pass a float value (0.0) for the min value as a vararg value for an int property (this is does right in the g_property_set_default() example)?

What about allowing subclasses not only to override the default, but also to further constrain the ranges. One example - In gstreamer we have a videsink that API wise supports width,height from 1 ... 65536 pixels (purely fictional). When we actually open a specific device, we read the hardware capabilities and like to constrain the limits to e.g. 4 ... 4096. The range can only be constrained and never expanded.

http://git.gnome.org/browse/glib/tree/gobject/gproperty.c?h=wip/gproperty#n331
For custom accessors, how does validation works? Maybe also tell that the framework does the notify-signal (if the setter returns TRUE) - if that is the case.

I really like this. Great job!
Comment 43 Dan Winship 2012-10-18 19:13:25 UTC
(In reply to comment #18)
> The reason I'm asking is that I'd like to keep the initial merge small,
> otherwise we'll get stuck bikeshedding over details for too long and nothing
> ever happens...

who, us?


thoughts on the branch:

- Don't we need/want G_PROPERTY_CONSTRUCT_ONLY?

- Having all the GProperty constructors take exactly the same arguments seems like a holdover from the earlier macro-based version of things which no longer really makes sense. Why *not* have g_int_property_new() take min/max/default? (Well, other than "because I can never remember if default comes first or third"...) And especially, why not have g_boxed_property_new() take the specific boxed GType, rather than having a mandatory followup call to g_property_set_prerequisite().

- if the "best practices" are going to still involve having a PROP_xxx enum and a static GParamSpec array, then we should require the array name to be t_n##_properties, and then pass an enum value to G_DEFINE_PROPERTY_GET_SET() so it can grab the GProperty directly from the array rather than having to call g_object_class_find_property(). (Or is g_object_class_find_property() fast enough that we don't need to care?)

- and maybe the static GParamSpec array should be a GProperty array (with g_*_property_new() returning GProperty* rather than GParamSpec*), because in the example code, you want them to be GProperty* more often than you want them to be GParamSpec*.

- The generated get/set implementations should g_assert() that the property is readable/writable, not warn and return a default value. It's a programmer error to provide a public getter function for a non-readable property or a public setter for a non-writable property (and a double-programmer-error to *call* such a function).
Comment 44 Emmanuele Bassi (:ebassi) 2012-10-19 08:28:00 UTC
(In reply to comment #43)
> (In reply to comment #18)
> > The reason I'm asking is that I'd like to keep the initial merge small,
> > otherwise we'll get stuck bikeshedding over details for too long and nothing
> > ever happens...
> 
> who, us?

yeah, bikeshedding? that never happens.

> thoughts on the branch:
> 
> - Don't we need/want G_PROPERTY_CONSTRUCT_ONLY?

after discussing it at the last GTK+ hackfest, construct-only was deemed a mistake; you can set the default value inside init() (though in theory you want that to happen automatically, really).

> - Having all the GProperty constructors take exactly the same arguments seems
> like a holdover from the earlier macro-based version of things which no longer
> really makes sense. Why *not* have g_int_property_new() take min/max/default?

no, it's intended. the API in GParamSpec is awful to remember (which one gets an additional parameter? which one gets a min/max/default? which one gets a type and a value?).

that's also the reason why the properties have default min/max/default values and the property types are C types as well as GTypes - like g_int8/16/32_property_new. you should say you want an uint8 property, instead of saying: "I want an int property between 0 and 255".

> (Well, other than "because I can never remember if default comes first or
> third"...) And especially, why not have g_boxed_property_new() take the
> specific boxed GType, rather than having a mandatory followup call to
> g_property_set_prerequisite().

because I want to avoid an asymmetry there.

> - if the "best practices" are going to still involve having a PROP_xxx enum and
> a static GParamSpec array, then we should require the array name to be
> t_n##_properties, and then pass an enum value to G_DEFINE_PROPERTY_GET_SET() so
> it can grab the GProperty directly from the array rather than having to call
> g_object_class_find_property(). (Or is g_object_class_find_property() fast
> enough that we don't need to care?)

that's a good point.

> - and maybe the static GParamSpec array should be a GProperty array (with
> g_*_property_new() returning GProperty* rather than GParamSpec*), because in
> the example code, you want them to be GProperty* more often than you want them
> to be GParamSpec*.

yeah.

> - The generated get/set implementations should g_assert() that the property is
> readable/writable, not warn and return a default value. It's a programmer error
> to provide a public getter function for a non-readable property or a public
> setter for a non-writable property (and a double-programmer-error to *call*
> such a function).

another good point.

by the by, the missing issues that I couldn't figure out to fix and that Ryan and I discussed at Brno earlier in 2012 are:

  - finding the instance private data offset at run time, in a way that is not horribly inefficient or that doesn't rely on (idiomatic but) flaky assumptions like "the last pointer-sized slot in the instance structure is a pointer to the private data structure";
  - interface overriding, using the setter/getter functions mapped to corresponding interface virtual functions.

the second item is still debatable - until implementing interfaces requires overriding all the properties I don't think we should encourage properties on interfaces, and even then mapping to virtual functions is pretty verbose - but the first item is a deal-breaker for inclusion. you can start from g_type_instance_get_private(), but that function is fairly expensive, and the reason we store a pointer to the private data structure in the instance data structure in the first place. we could have a g_type_instance_set_private_offset(g_define_id, G_STRUCT_OFFSET (Instance, priv)) called inside G_DEFINE_TYPE, e.g.:

  G_DEFINE_TYPE_WITH_CODE (Foo, foo, G_TYPE_OBJECT,
                           G_PRIVATE (Foo, priv, FooPrivate))

which also calls g_type_class_add_private().
Comment 45 Dan Winship 2012-10-19 09:41:53 UTC
(In reply to comment #44)
> > - Don't we need/want G_PROPERTY_CONSTRUCT_ONLY?
> 
> after discussing it at the last GTK+ hackfest, construct-only was deemed a
> mistake; you can set the default value inside init() (though in theory you want
> that to happen automatically, really).

That's G_PARAM_CONSTRUCT, which I agree is unnecessary. But G_PARAM_CONSTRUCT_ONLY is important for when you *don't* want a default value at construct time, but it would either be silly or incredibly complicated to allow users to change the value after the object is already in use. I guess you could say "don't use a property in that case", but that makes things more complicated for bindings.

> we could have a
> g_type_instance_set_private_offset(g_define_id, G_STRUCT_OFFSET (Instance,
> priv)) called inside G_DEFINE_TYPE, e.g.:
> 
>   G_DEFINE_TYPE_WITH_CODE (Foo, foo, G_TYPE_OBJECT,
>                            G_PRIVATE (Foo, priv, FooPrivate))
> 
> which also calls g_type_class_add_private().

and then if you'd called that on a type, g_object_init() or g_object_constructor() or something could initialize the priv pointer for you automatically...

And the macro should just assume "priv" and "c_n##Private"; if there are people with legacy types with different naming conventions, they can just not use G_PRIVATE.
Comment 46 Emmanuele Bassi (:ebassi) 2012-10-19 10:01:41 UTC
(In reply to comment #45)

> > we could have a
> > g_type_instance_set_private_offset(g_define_id, G_STRUCT_OFFSET (Instance,
> > priv)) called inside G_DEFINE_TYPE, e.g.:
> > 
> >   G_DEFINE_TYPE_WITH_CODE (Foo, foo, G_TYPE_OBJECT,
> >                            G_PRIVATE (Foo, priv, FooPrivate))
> > 
> > which also calls g_type_class_add_private().
> 
> and then if you'd called that on a type, g_object_init() or
> g_object_constructor() or something could initialize the priv pointer for you
> automatically...

yes, that would be good.
 
> And the macro should just assume "priv" and "c_n##Private"; if there are people
> with legacy types with different naming conventions, they can just not use
> G_PRIVATE.

or use a local typedef, like we do with G_DEFINE_INTERFACE.
Comment 47 David Zeuthen (not reading bugmail) 2012-10-20 00:31:32 UTC
(In reply to comment #45)
> But
> G_PARAM_CONSTRUCT_ONLY is important for when you *don't* want a default value
> at construct time, but it would either be silly or incredibly complicated to
> allow users to change the value after the object is already in use.

Real-world example: GDBusConnection and its :address and :stream properties which are both CONSTRUCT_ONLY and where you set precisely one of them at construction (never both). Here it doesn't even make *sense* to write to these properties post-construction. So deprecating CONSTRUCT_ONLY is not an option.
Comment 48 Dan Winship 2013-04-05 21:12:07 UTC
a few notes while trying to port some things to GProperty...


It is weird that g_property_set_default() does not automatically cause the property to have that default value. OK, GParamSpec works the same way (for non-CONSTRUCT properties), but it always seemed weird there too. The ability to introspect the default value is only interesting if that value *really is* the default, so why even make it possible for the programmer to get it wrong?


g_property_set_prerequisite() sounds very technical and doesn't describe what it does very well. Maybe g_property_set_specific_type()?


G_DEFINE_PROPERTY_SET_WITH_CODE() and G_DEFINE_PROPERTY_GET_WITH_CODE() seem like something we don't actually want, because they would make the direct setter/getter behave differently from g_object_set/get(), and I don't think there's any good reason to do that.


One thing I've been disappointed by so far is the number of times the property name ends up appearing in its definition. Eg:

  g_socket_properties[PROP_FAMILY] =
    g_enum_property_new ("family", G_PROPERTY_READWRITE /* | G_PROPERTY_CONSTRUCT_ONLY */,
                         G_STRUCT_OFFSET (GSocketPrivate, family),
                         NULL, NULL);
  g_property_set_prerequisite (G_PROPERTY (g_socket_properties[PROP_FAMILY]),
                               G_TYPE_SOCKET_FAMILY);
  g_property_set_default (G_PROPERTY (g_socket_properties[PROP_FAMILY]),
                          G_SOCKET_FAMILY_INVALID);

If I then want to copy+paste that to create the nearly-identical GSocket:type and GSocket:protocol properties, I have to change "family" to "type"/"protocol" in 5 places (not counting G_TYPE_SOCKET_FAMILY and G_SOCKET_FAMILY_INVALID). It's really easy to end up missing one of them, and then, eg, accidentally setting the default value of the wrong property.

Some of the earlier, more heavily-magic, versions of the API would avoid this, but I don't remember what other problems they had...

(I guess one thing that might help is using a temporary variable, changing the first line to

  g_socket_properties[PROP_FAMILY] = prop =
    ...

and then just using "prop" rather than "g_socket_properties[PROP_FAMILY]" on the next two lines.)
Comment 49 Emmanuele Bassi (:ebassi) 2013-04-05 22:25:29 UTC
(In reply to comment #48)
> a few notes while trying to port some things to GProperty...

thanks Dan, it's very much appreciated. I'll try getting this merged at the hackfest, it's getting pretty ridiculous.

> It is weird that g_property_set_default() does not automatically cause the
> property to have that default value. OK, GParamSpec works the same way (for
> non-CONSTRUCT properties), but it always seemed weird there too. The ability to
> introspect the default value is only interesting if that value *really is* the
> default, so why even make it possible for the programmer to get it wrong?

the main issue is that we don't have an instance when we register the property; the only way to avoid it would be to have some piece of code inside GObject that automatically assigns the default values — something that mclasen proposed as well. my intention was to limit the amount of changes in GObject itself to a minimum, but I think this case warrants a change.
 
> g_property_set_prerequisite() sounds very technical and doesn't describe what
> it does very well. Maybe g_property_set_specific_type()?

it's using the same terminology we use for interfaces.

> G_DEFINE_PROPERTY_SET_WITH_CODE() and G_DEFINE_PROPERTY_GET_WITH_CODE() seem
> like something we don't actually want, because they would make the direct
> setter/getter behave differently from g_object_set/get(), and I don't think
> there's any good reason to do that.

well, strictly speaking, those are the two functions that g_object_set/get would end up calling, and we have plenty of property setters that have side effects like notification on multiple properties already. I almost agree for the getter case, though.

> One thing I've been disappointed by so far is the number of times the property
> name ends up appearing in its definition. Eg:
> 
>   g_socket_properties[PROP_FAMILY] =
>     g_enum_property_new ("family", G_PROPERTY_READWRITE /* |
> G_PROPERTY_CONSTRUCT_ONLY */,
>                          G_STRUCT_OFFSET (GSocketPrivate, family),
>                          NULL, NULL);
>   g_property_set_prerequisite (G_PROPERTY (g_socket_properties[PROP_FAMILY]),
>                                G_TYPE_SOCKET_FAMILY);
>   g_property_set_default (G_PROPERTY (g_socket_properties[PROP_FAMILY]),
>                           G_SOCKET_FAMILY_INVALID);
> 
> If I then want to copy+paste that to create the nearly-identical GSocket:type
> and GSocket:protocol properties, I have to change "family" to "type"/"protocol"
> in 5 places (not counting G_TYPE_SOCKET_FAMILY and G_SOCKET_FAMILY_INVALID).
> It's really easy to end up missing one of them, and then, eg, accidentally
> setting the default value of the wrong property.

this is a fair complaint.

> Some of the earlier, more heavily-magic, versions of the API would avoid this,
> but I don't remember what other problems they had...

the main problem was the overall automagic nature of it all, but I'd seriously consider a G_DEFINE_PROPERTY macro that just did:

  G_DEFINE_PROPERTY (enum, g_socket_properties[PROP_FAMILY], "family",
                     G_STRUCT_OFFSET (GSocketPrivate, family),
                     G_PROPERTY_READWRITE)

and a WITH_CODE variant that worked like G_DEFINE_TYPE_WITH_CODE; then we'd just need macros like G_PROPERTY_RANGE, G_PROPERTY_DEFAULT, G_PROPERTY_DESCRIBE, etc.

I'll try to come up with something decent this weekend.
Comment 50 Dan Winship 2013-04-06 14:27:14 UTC
(In reply to comment #49)
> > g_property_set_prerequisite() sounds very technical and doesn't describe what
> > it does very well. Maybe g_property_set_specific_type()?
> 
> it's using the same terminology we use for interfaces.

It's not the same thing though. An interface prerequisite is a type that you have to implement before you can implement the interface. This isn't that. Eg:

> >   g_socket_properties[PROP_FAMILY] =
> >     g_enum_property_new ("family", G_PROPERTY_READWRITE /* |
> > G_PROPERTY_CONSTRUCT_ONLY */,
> >                          G_STRUCT_OFFSET (GSocketPrivate, family),
> >                          NULL, NULL);
> >   g_property_set_prerequisite (G_PROPERTY (g_socket_properties[PROP_FAMILY]),
> >                                G_TYPE_SOCKET_FAMILY);

G_TYPE_SOCKET_FAMILY isn't a "prerequisite" of the property, it's the actual type of the property.
Comment 51 Emmanuele Bassi (:ebassi) 2013-06-11 13:25:12 UTC
Created attachment 246511 [details] [review]
gobject: Add GProperty / v3.0

Cleaned up version of GProperty.

I'm still not entirely sure of the "atomic" flag, especially as it requires some minor feature in GObject to lock the instance on a per-property base, and it is currently untested.
Comment 52 Emmanuele Bassi (:ebassi) 2013-06-11 13:26:19 UTC
Created attachment 246512 [details] [review]
gproperty: Add default values

GProperty should store the default value (if any) of a property, as well as the eventual overrides coming from the sub-classes of the class that defined the property.

This is mostly like the newly added GParamSpec default value, but it allows overriding from subclasses.
Comment 53 Emmanuele Bassi (:ebassi) 2013-06-11 13:26:33 UTC
Created attachment 246513 [details] [review]
gobject: Use GProperty varargs API when accessing properties

Instead of using GValue, let's take advantage of the fast paths with no
boxing/unboxing inside GProperty.
Comment 54 Emmanuele Bassi (:ebassi) 2013-06-11 13:27:28 UTC
Created attachment 246514 [details] [review]
property: Add autogeneration of accessors

We can use macros to autogenerate accessor methods for GObject classes using GProperty.

There are also macros to define properties inside class_init(), but I'm less certain about them: they look a bit iffy to me, even if they *do* save time and code.
Comment 55 Emmanuele Bassi (:ebassi) 2013-06-11 13:27:43 UTC
Created attachment 246515 [details] [review]
gparam: Add setters for static nick and blurb

We are going to need them for GProperty.
Comment 56 Emmanuele Bassi (:ebassi) 2013-06-11 13:28:12 UTC
Created attachment 246516 [details] [review]
gproperty: Add macros for defining properties

Alongside the macros for defining accessors we may want to use simple macros for defining properties to install on a GObject class.

I'm not entirely enamoured with these macros.
Comment 57 Emmanuele Bassi (:ebassi) 2013-06-11 13:29:13 UTC
Created attachment 246518 [details] [review]
gobject: Add quark-based locking

Private, because it's a bit weird; I would like to re-implement it using some high bit somewhere, if we have a spare one, instead of using a GData to store an allocated integer.
Comment 58 Emmanuele Bassi (:ebassi) 2013-06-11 13:29:55 UTC
Created attachment 246519 [details] [review]
property: Simplify code generation and private data access

The macros (both internal and external) and the accessors can be simplified by the new private data memory layout, as well as by cleaning up some redundant checks.

This should probably be squashed.
Comment 59 Emmanuele Bassi (:ebassi) 2013-06-11 13:31:44 UTC
Created attachment 246520 [details] [review]
gobject: Initialize GProperties at instance init time

Using their default value. This removes the need to manually calling g_property_init_default(), which means we can make it private.

The call to the setters happen before the actual instance init() has been called, and thus could be problematic if the setter relies on some state set up by the instance init(), like, say, the priv pointer inside the instance data structure.
Comment 60 Dan Winship 2013-06-12 21:09:03 UTC
Haven't tried using the new code yet, but it looks good on paper.


The last two commits seem like they should be squashed back into the appropriate earlier commits. (Yeah, I know why they're currently separate, but that doesn't matter any more.)

G_PROPERTY_ATOMIC properties need to do locking on read too, or there's no guarantee that the writes will actually be seen atomically. (Without at least a memory barrier, they may not be seen at all...)

g_flags_property_set_value() and g_enum_property_set_value() look like they'll do exciting and bad things if the enum type is not sizeof(long)

G_DEFINE_PROPERTY_SET_WITH_CODE() should clarify that the provided code only gets run if the property changed value (and that it runs after the ::notify is emitted?). (And g_property_set_va() should clarify that it returns FALSE if the property already had the provided value, and g_property_set() should document its return value at all).

And G_DEFINE_PROPERTY_GET_WITH_CODE() should note that the return value is in "retval"? I'm not totally sure what sort of post-processing you'd want to do in a getter, but it seems like you'd probably want access to the return value.

Not sure I like "COMPUTED_GET" as a name... the direct version is arguably "computed" too (priv + offset). I don't have a good alternative suggestion though, other than making the "computed" version be just GET and the direct version be GET_DIRECT. (But that would probably imply changing what G_DEFINE_PROPERTY_GET_SET() means too...)
Comment 61 Emmanuele Bassi (:ebassi) 2013-06-12 22:27:19 UTC
(In reply to comment #60)
> Haven't tried using the new code yet, but it looks good on paper.

apart from the tests/property test suite, I've started using the property accessors definition in my clutter-inside-gtk branch, and the amount of lines saved is pretty high already.
 
> The last two commits seem like they should be squashed back into the
> appropriate earlier commits. (Yeah, I know why they're currently separate, but
> that doesn't matter any more.)

once we actually decide, I'll probably squash the whole thing.

> G_PROPERTY_ATOMIC properties need to do locking on read too, or there's no
> guarantee that the writes will actually be seen atomically. (Without at least
> memory barrier, they may not be seen at all...)

the ATOMIC stuff is pretty much experimental, and I'd be willing to cut it out from the main commit and have it land afterwards. it's also untested, which is not very nice, plus it depends on that piece of trickery that is the quark-based locking inside GObject. in principle, it's fairly useful, especially to get the basics of property access under a lock right without the developers necessarily having to care — and one thing less to care about is one thing less to screw up.

> g_flags_property_set_value() and g_enum_property_set_value() look like they'll
> do exciting and bad things if the enum type is not sizeof(long)

we always assume that any G_TYPE_ENUM is <= sizeof(glong) and that any G_TYPE_FLAGS is <= sizeof(gulong) anyway.

> G_DEFINE_PROPERTY_SET_WITH_CODE() should clarify that the provided code only
> gets run if the property changed value (and that it runs after the ::notify is
> emitted?). (And g_property_set_va() should clarify that it returns FALSE if the
> property already had the provided value, and g_property_set() should document
> its return value at all).

true, missed that.

> And G_DEFINE_PROPERTY_GET_WITH_CODE() should note that the return value is in
> "retval"? I'm not totally sure what sort of post-processing you'd want to do in
> a getter, but it seems like you'd probably want access to the return value.

I'd probably rename it as g_property_value or something, so at least it's namespaced.
 
> Not sure I like "COMPUTED_GET" as a name... the direct version is arguably
> "computed" too (priv + offset). I don't have a good alternative suggestion
> though, other than making the "computed" version be just GET and the direct
> version be GET_DIRECT. (But that would probably imply changing what
> G_DEFINE_PROPERTY_GET_SET() means too...)

yes, COMPUTED_GET was a suggestion from Ryan, but I'm up to other names. DIRECT_GET would be okay to me, but as you note that would have some sort of cascade effect on the GET_SET macro. maybe INDIRECT_GET instead of COMPUTED_GET?
Comment 62 Benjamin Otte (Company) 2013-06-12 23:21:05 UTC
(In reply to comment #61)
> the ATOMIC stuff is pretty much experimental, and I'd be willing to cut it out
> from the main commit and have it land afterwards. it's also untested, which is
> not very nice, plus it depends on that piece of trickery that is the
> quark-based locking inside GObject.
>
I vote leaving it out now. That makes (a) the current patch set easier to review and (b) we can add it when we actually have a use case for it. It's always a bad idea to add APIs just because after all...
Comment 63 Dan Winship 2013-06-13 03:57:58 UTC
(In reply to comment #61)
> once we actually decide, I'll probably squash the whole thing.

that works too

> > g_flags_property_set_value() and g_enum_property_set_value() look like they'll
> > do exciting and bad things if the enum type is not sizeof(long)
> 
> we always assume that any G_TYPE_ENUM is <= sizeof(glong) and that any
> G_TYPE_FLAGS is <= sizeof(gulong) anyway.

I was worried about enums smaller than long, not larger; the gets and sets are always done via pointers-to-long, so if the enum is smaller than that (ie, sizeof(int) on x86_64), you'll end up reading/overwriting part or all of the next field as well.
Comment 64 Emmanuele Bassi (:ebassi) 2013-06-13 11:08:35 UTC
(In reply to comment #63)
> > > g_flags_property_set_value() and g_enum_property_set_value() look like they'll
> > > do exciting and bad things if the enum type is not sizeof(long)
> > 
> > we always assume that any G_TYPE_ENUM is <= sizeof(glong) and that any
> > G_TYPE_FLAGS is <= sizeof(gulong) anyway.
> 
> I was worried about enums smaller than long, not larger; the gets and sets are
> always done via pointers-to-long, so if the enum is smaller than that (ie,
> sizeof(int) on x86_64), you'll end up reading/overwriting part or all of the
> next field as well.

ugh, it's even worse than that.

GEnum and GFlags will collect GValues using int-sized addresses, but store the value into long-sized storage — that's what threw me off.

(for the record, bug 663054 is still open for adding 64-bit wide enumeration and flags GTypes)

I'll re-work the enums and flags, and use gint/guint instead of glong/gulong.
Comment 65 Emmanuele Bassi (:ebassi) 2013-06-13 11:10:07 UTC
(In reply to comment #62)
> (In reply to comment #61)
> > the ATOMIC stuff is pretty much experimental, and I'd be willing to cut it out
> > from the main commit and have it land afterwards. it's also untested, which is
> > not very nice, plus it depends on that piece of trickery that is the
> > quark-based locking inside GObject.
> >
> I vote leaving it out now. That makes (a) the current patch set easier to
> review and (b) we can add it when we actually have a use case for it. It's
> always a bad idea to add APIs just because after all...

I still think it would be a good thing to have atomic properties sooner, rather than later; ${DEITY} only knows how many people get that crap wrong, and we're seeing a pretty definite uptake in thread-based GObject code all over the platform.
Comment 66 Benjamin Otte (Company) 2013-06-13 11:48:43 UTC
I still think we should have 2 kinds of objects: Threadsafe ones and non-threadsafe ones. Because nobody ever makes dispose, finalize and g_signal_connect() calls threadsafe...
Comment 67 Emmanuele Bassi (:ebassi) 2013-06-13 17:47:25 UTC
Created attachment 246754 [details] [review]
gobject: Add GProperty / v3.1

changes:

  - removed the atomic/lock code
  - enums/flags are using ints/unsigned ints
  - squashed a bunch of changes
Comment 68 Emmanuele Bassi (:ebassi) 2013-06-13 17:47:48 UTC
Created attachment 246755 [details] [review]
gproperty: Add default values

GProperty should store the default value (if any) of a property, as well
as the eventual overrides coming from the sub-classes of the class that
defined the property.
Comment 69 Emmanuele Bassi (:ebassi) 2013-06-13 17:48:04 UTC
Created attachment 246756 [details] [review]
gobject: Use GProperty varargs API when accessing properties

Instead of using GValue, let's take advantage of the fast paths with no
boxing/unboxing inside GProperty.
Comment 70 Emmanuele Bassi (:ebassi) 2013-06-13 17:48:40 UTC
Created attachment 246757 [details] [review]
property: Add autogeneration of accessors

We can use macros to autogenerate accessor methods for GObject classes
using GProperty.
Comment 71 Emmanuele Bassi (:ebassi) 2013-06-13 17:48:55 UTC
Created attachment 246758 [details] [review]
gparam: Add setters for static nick and blurb

We are going to need them for GProperty.
Comment 72 Emmanuele Bassi (:ebassi) 2013-06-13 17:49:15 UTC
Created attachment 246759 [details] [review]
gobject: Initialize GProperties at instance init time

Using their default value. This removes the need to manually calling
g_property_init_default(), which means we can make it private.
Comment 73 Emmanuele Bassi (:ebassi) 2013-06-13 17:49:51 UTC
Created attachment 246760 [details] [review]
gproperty: Add macros for defining properties

Alongside the macros for defining accessors we may want to use simple
macros for defining properties to install on a GObject class.
Comment 74 Emmanuele Bassi (:ebassi) 2013-06-14 15:01:56 UTC
Created attachment 246820 [details] [review]
gproperty: Add macros for defining properties

another version of attachment 246760 [details] [review] - this one defines a couple of functions in G_DECLARE_PROPERTIES that can be reused in the G_DEFINE_PROPERTY_GET/SET macros to find properties without going through the GObject class in the common case. Also, it adds a safe wrapper around g_object_notify_by_pspec() for the current class, for side-effect notifications.

personally, I like this patch best: it ties everything together with a nice bow.
Comment 75 Dan Winship 2013-06-14 18:31:10 UTC
I think you can get rid of G_DECLARE_PROPERTY(). Maybe even G_DECLARE_PROPERTIES().

Nothing cares about the order of the _properties[] array's elements, so it doesn't matter what order the GProperty*s get added to it, so instead of inserting each one at [PROP_##T_n##_##name], you could just insert each one at [counter++].

At that point, nothing would ever use any value of the PROP_ enum except _LAST, and so the G_DECLARE_PROPERTY() calls would serve no purpose except for causing PROP_type_LAST to get the right value. But you could get rid of the need to know _LAST by making the _properties array be dynamically-allocated and NULL-terminated rather than static and fixed-length; G_DEFINE_PROPERTIES() could build it up via a local GPtrArray and then assign the array to t_n##_properties at the end.

You don't need t_n##_class_install_properties() really either; you could just expand that inline at the end of G_DEFINE_PROPERTIES. And you don't really need per-type t_n##_find_property() and _notify_property(); just have a generic version of each that takes a _properties array as one of its parameters.

And then assuming all the above, G_DECLARE_PROPERTIES() is reduced to:

+#define G_DECLARE_PROPERTIES(T_N, t_n) \
+static GProperty **t_n##_properties;

and really, with the addition of a G_GNUC_UNUSED, we could maybe just stick that into G_DEFINE_TYPE...
Comment 76 Alexander Larsson 2013-06-17 10:30:37 UTC
The docs still say:

 * For direct access to the private structure
 * members, #GProperty will assume that the instance structure contains a
 * pointer to the private data structure as its last member, e.g.:
 *
Comment 77 Alexander Larsson 2013-06-17 10:49:56 UTC
Also, get_private() calls get_type() before its defined, so if no header 
declares it it will break. This needs to either declare it or switch the order of the functions. This breaks gtk atm:

gdkkeys-broadway.c: In function 'gdk_broadway_keymap_get_private':
gdkkeys-broadway.c:59:1: warning: implicit declaration of function 'gdk_broadway_keymap_get_type' [-Wimplicit-function-declaration]
gdkkeys-broadway.c: At top level:
gdkkeys-broadway.c:59:1037: error: conflicting types for 'gdk_broadway_keymap_get_type'
gdkkeys-broadway.c:59:995: note: previous implicit declaration of 'gdk_broadway_keymap_get_type' was here
Comment 78 Alexander Larsson 2013-06-17 10:52:31 UTC
Review of attachment 246820 [details] [review]:

::: gobject/gproperty.h
@@ +776,3 @@
+ * @flags: #GPropertyFlags for the property
+ * @minVal: the minimum value of the property
+ * @maxVal: the maximum value of the property

no min/maxVal here
Comment 79 Benjamin Otte (Company) 2013-06-17 10:55:31 UTC
(In reply to comment #77)
> Also, get_private() calls get_type() before its defined, so if no header 
> declares it it will break. This needs to either declare it or switch the order
> of the functions. This breaks gtk atm:
> 
> gdkkeys-broadway.c: In function 'gdk_broadway_keymap_get_private':
> gdkkeys-broadway.c:59:1: warning: implicit declaration of function
> 'gdk_broadway_keymap_get_type' [-Wimplicit-function-declaration]
> gdkkeys-broadway.c: At top level:
> gdkkeys-broadway.c:59:1037: error: conflicting types for
> 'gdk_broadway_keymap_get_type'
> gdkkeys-broadway.c:59:995: note: previous implicit declaration of
> 'gdk_broadway_keymap_get_type' was here
>
You cannot declare it because that'll give you a warning about double declaration when you include the header that you should be including in the first place. Why doesn't gdkkeys-broadway.c do that btw? Any reason other than it not being necessary/having forgotten?
Comment 80 Emmanuele Bassi (:ebassi) 2013-06-17 11:52:47 UTC
(In reply to comment #75)
> I think you can get rid of G_DECLARE_PROPERTY(). Maybe even
> G_DECLARE_PROPERTIES().
> 
> Nothing cares about the order of the _properties[] array's elements, so it
> doesn't matter what order the GProperty*s get added to it, so instead of
> inserting each one at [PROP_##T_n##_##name], you could just insert each one at
> [counter++].
> 
> At that point, nothing would ever use any value of the PROP_ enum except _LAST,
> and so the G_DECLARE_PROPERTY() calls would serve no purpose except for causing
> PROP_type_LAST to get the right value. But you could get rid of the need to
> know _LAST by making the _properties array be dynamically-allocated and
> NULL-terminated rather than static and fixed-length; G_DEFINE_PROPERTIES()
> could build it up via a local GPtrArray and then assign the array to
> t_n##_properties at the end.

not a huge fan of this: an array of pointers in static storage is one thing, an allocation is a bit different.

> You don't need t_n##_class_install_properties() really either; you could just
> expand that inline at the end of G_DEFINE_PROPERTIES.

in theory, yes. though one thing I'd like to see is being able to move most of this stuff (even GType data at some point) into static storage.

> And you don't really need
> per-type t_n##_find_property() and _notify_property(); just have a generic
> version of each that takes a _properties array as one of its parameters.

that's not really nice. the whole point is to provide a notify() that takes a const char* but that doesn't end up hitting the GParamSpecPool in the very common case. that's the whole point of storing the GParamSpec of a class inside an array in the first place.

> And then assuming all the above, G_DECLARE_PROPERTIES() is reduced to:
> 
> +#define G_DECLARE_PROPERTIES(T_N, t_n) \
> +static GProperty **t_n##_properties;
> 
> and really, with the addition of a G_GNUC_UNUSED, we could maybe just stick
> that into G_DEFINE_TYPE...

I see your point. I don't particularly like it, though.

anyway, I'm willing to fix up the branch if others agree.
Comment 81 Emmanuele Bassi (:ebassi) 2013-06-17 11:56:31 UTC
(In reply to comment #76)
> The docs still say:
> 
>  * For direct access to the private structure
>  * members, #GProperty will assume that the instance structure contains a
>  * pointer to the private data structure as its last member, e.g.:
>  *

thanks, noted.

(In reply to comment #77)
> Also, get_private() calls get_type() before its defined, so if no header 
> declares it it will break. This needs to either declare it or switch the order
> of the functions. This breaks gtk atm:
> 
> gdkkeys-broadway.c: In function 'gdk_broadway_keymap_get_private':
> gdkkeys-broadway.c:59:1: warning: implicit declaration of function
> 'gdk_broadway_keymap_get_type' [-Wimplicit-function-declaration]
> gdkkeys-broadway.c: At top level:
> gdkkeys-broadway.c:59:1037: error: conflicting types for
> 'gdk_broadway_keymap_get_type'
> gdkkeys-broadway.c:59:995: note: previous implicit declaration of
> 'gdk_broadway_keymap_get_type' was here

that's a bit unfortunate: I cannot add the get_private() function after the get_type() function has been defined, because that would mean changing the _G_DEFINE_TYPE_EXTENDED_END() macro, and that macro is used for other G_DEFINE_* macros that have nothing to do with GObject. I'd have to add a new macro to close off the get_type() function.

in general, the get_type() function is declared inside the header that defines the type, though; for internal API, I usually always declare the get_type() function anyway before calling G_DEFINE_TYPE.

(In reply to comment #78)
> Review of attachment 246820 [details] [review]:
> 
> ::: gobject/gproperty.h
> @@ +776,3 @@
> + * @flags: #GPropertyFlags for the property
> + * @minVal: the minimum value of the property
> + * @maxVal: the maximum value of the property
> 
> no min/maxVal here

noted.
Comment 82 Alexander Larsson 2013-06-17 15:18:41 UTC
> You cannot declare it because that'll give you a warning about double
> declaration when you include the header that you should be including in the
> first place. Why doesn't gdkkeys-broadway.c do that btw? Any reason other than
> it not being necessary/having forgotten?

Are double declarations really warnings?
Comment 83 Benjamin Otte (Company) 2013-06-17 15:29:45 UTC
They can be with the right gcc warning flag, I think its -Wredundant-decls
Comment 84 Emmanuele Bassi (:ebassi) 2013-06-17 15:59:46 UTC
pushed wip/gproperty-2 to git.gnome.org; the last commit on that branch implements what Dan outlined in comment 75: no G_DECLARE_PROPERT(Y|IES), property definition goes through a GPtrArray, and G_DEFINE_TYPE_EXTENDED gets a GParamSpec* array (annotated with G_GNUC_UNUSED for the common case).

instead of calling g_object_class_find_property() inside the macro-generated setter and getter functions, I go through the pspec array using the interned canonicalized name to match it; shouldn't be excessively slower than g_param_spec_pool_lookup(), given the average size of the array, and considering that it's just a pointer comparison.

I should start looking at porting some code in GObject/GIO to the macros, to see what happens there; may even be interesting to look at gdbus-codegen.
Comment 85 Emmanuele Bassi (:ebassi) 2013-06-17 18:19:08 UTC
(In reply to comment #77)
> Also, get_private() calls get_type() before its defined, so if no header 
> declares it it will break. This needs to either declare it or switch the order
> of the functions.

added a commit that rejigs _G_DEFINE_TYPE_EXTENDED() for instance types to define the get_private() function after the get_type() one; not as awful as I feared. see bug 700035 (that I set as a dependency for this one).
Comment 86 Alexander Larsson 2013-06-18 09:17:23 UTC
glade-project.c: At top level:
glade-project.c:4570:1: error: no previous prototype for 'glade_project_properties' [-Werror=missing-prototypes]
glade-project.c:4570:1: error: 'glade_project_properties' redeclared as different kind of symbol
glade-project.c:209:2: note: previous declaration of 'glade_project_properties' was here
Comment 87 Alexander Larsson 2013-06-18 09:20:08 UTC
Ah, it was a name collision:
void glade_project_properties (GladeProject *project);
Comment 88 Alexander Larsson 2013-06-18 09:37:30 UTC
Created attachment 247118 [details] [review]
gproperties: Rename properties static variable to avoid collisions

We add a _gtype_ to the name as it was conficting with existing
names in gtk+ and glade (at least).
Comment 89 Alexander Larsson 2013-06-18 11:51:21 UTC
I looked through the patches, and it looks generally good to me. Comments:

The docs are nice an extensive, but they don't use the new macros which we probably want most people to use. Also it doesn't use G_ADD_PRIVATE()

Do we really need both priv_offset and field_offset. Seems like these could be combined into a single offset to get at the field. We never use the priv for anything else.

g_property_set_default_value_internal & co, why are we hashing on the type name, we should do a direct hash on the GType instead.

There is no standard support for GVariant properties, or GUnichar. Not sure if these are interesting...
Comment 90 Emmanuele Bassi (:ebassi) 2013-06-18 14:11:54 UTC
(In reply to comment #89)
> I looked through the patches, and it looks generally good to me. Comments:
> 
> The docs are nice an extensive, but they don't use the new macros which we
> probably want most people to use. Also it doesn't use G_ADD_PRIVATE()

yes, the documentation was written before the macros. I'll do a pass and rework it to incorporate them.

> Do we really need both priv_offset and field_offset. Seems like these could be
> combined into a single offset to get at the field. We never use the priv for
> anything else.

that is true.

> g_property_set_default_value_internal & co, why are we hashing on the type
> name, we should do a direct hash on the GType instead.

if we hash on the GType we need to add a new hashing function so that we are 64 bit aware; Colin had to add a similar hash table in gjs recently.

> There is no standard support for GVariant properties, or GUnichar. Not sure if
> these are interesting...

GUnichar is supported using a typedef to guint32, alongside another couple of identities:

#define g_char_property_new     g_int8_property_new
#define g_uchar_property_new    g_uint8_property_new
#define g_unichar_property_new  g_uint32_property_new

GVariant is just missing — I could make g_property_set_prerequisite() work like g_property_set_default(), and then we could use it to specify the GVariantType.
Comment 91 Alexander Larsson 2013-06-18 14:49:21 UTC
> if we hash on the GType we need to add a new hashing function so that we are 64
> bit aware; Colin had to add a similar hash table in gjs recently.

GType is a GSize, so 64bit should only be needed on 64bit arches. Should work with g_direct_hash.
Comment 92 Colin Walters 2013-06-18 14:52:46 UTC
(In reply to comment #91)
> > if we hash on the GType we need to add a new hashing function so that we are 64
> > bit aware; Colin had to add a similar hash table in gjs recently.
> 
> GType is a GSize, so 64bit should only be needed on 64bit arches. Should work
> with g_direct_hash.

Yeah, x32 is the ugly duckling: http://en.wikipedia.org/wiki/X32_ABI
gpointer is 32 bits, gsize is 64.

Hash table in question is in https://git.gnome.org/browse/gjs/commit/?id=8eae3f9fca09f4acb8b295e78e86bee65bd97a0f
Comment 93 Emmanuele Bassi (:ebassi) 2013-06-18 15:09:14 UTC
I just realized that if we want to allow people to use the offset of the field regardless of whether it's from the instance structure or the instance private structure, then we need to have a macro that behaves like G_STRUCT_OFFSET but automatically adds the private offset defined by G_DEFINE_TYPE. this cascades into a set of changes:

  - type_name##_private_offset should become TypeName##_private_offset;
  - G_ADD_PRIVATE should only take the camel case TypeName;
  - G_PRIVATE_OFFSET (TypeName, field_name) would then evaluate to:
 
  TypeName##_private_offset + G_STRUCT_OFFSET (TypeName##Private, field_name)

then G_DEFINE_PROPERTY_EXTENDED can do:

  G_DEFINE_PROPERTY_EXTENDED (GtkGadget,
                              int,
                              width,
                              G_PRIVATE_OFFSET (GtkGadget, width),
                              NULL, NULL,
                              G_PROPERTY_READWRITE,
                              {})

for properties stored in the private data structure, or do:

  G_DEFINE_PROPERTY_EXTENDED (GtkGadget,
                              int32,
                              flags,
                              G_STRUCT_OFFSET (GtkGadget, flags),
                              NULL, NULL,
                              G_PROPERTY_READWRITE,
                              {})

for properties stored in the public data structure.

G_DEFINE_PROPERTY() and friends would implicitly use G_PRIVATE_OFFSET instead of G_STRUCT_OFFSET, and always assume that the property is stored in the private data structure, like the do right now.
Comment 94 Alexander Larsson 2013-06-19 09:29:15 UTC
X32 having a 64bit size_t is indeed insane. However, it does not affect GType. GTypes really *are* a pointer cast to GSize, so they will never actually be larger than a pointer value, so limiting it to 32bit is safe. I don't see any need for the gjs hashtable implementation, it could just use g_direct_hash.
Comment 95 Alexander Larsson 2013-06-19 10:12:24 UTC
G_PRIVATE_OFFSET Looks very sweet!
Comment 96 Colin Walters 2013-06-19 11:47:08 UTC
(In reply to comment #94)
> X32 having a 64bit size_t is indeed insane. However, it does not affect GType.
> GTypes really *are* a pointer cast to GSize, so they will never actually be
> larger than a pointer value, so limiting it to 32bit is safe. I don't see any
> need for the gjs hashtable implementation, it could just use g_direct_hash.

Right, duh.  Sorry for the noise.
Comment 97 Emmanuele Bassi (:ebassi) 2013-06-24 13:25:38 UTC
Created attachment 247635 [details] [review]
gobject: Add GProperty / v3.2
Comment 98 Emmanuele Bassi (:ebassi) 2013-06-24 13:26:06 UTC
Created attachment 247636 [details] [review]
gproperty: Add default values
Comment 99 Emmanuele Bassi (:ebassi) 2013-06-24 13:26:19 UTC
Created attachment 247637 [details] [review]
gobject: Use GProperty varargs API when accessing properties
Comment 100 Emmanuele Bassi (:ebassi) 2013-06-24 13:26:37 UTC
Created attachment 247639 [details] [review]
gobject: Initialize GProperties at instance creation
Comment 101 Emmanuele Bassi (:ebassi) 2013-06-24 13:26:49 UTC
Created attachment 247640 [details] [review]
gparam: Add setters for static nick and blurb

We are going to need them for GProperty.
Comment 102 Emmanuele Bassi (:ebassi) 2013-06-24 13:27:23 UTC
Created attachment 247641 [details] [review]
property: Add autogeneration of accessors
Comment 103 Emmanuele Bassi (:ebassi) 2013-06-24 13:27:42 UTC
Created attachment 247642 [details] [review]
property: Add macros for defining properties
Comment 104 Emmanuele Bassi (:ebassi) 2013-06-24 13:27:58 UTC
Created attachment 247643 [details] [review]
property: Use the field offset everywhere

Do not implicitly grab the private data offset inside GProperty: we
allow passing any offset when creating a new GProperty, and let the
user decide where the property is stored, by giving us the correct
offset — either from the instance structure, or from the private
structure.
Comment 105 Emmanuele Bassi (:ebassi) 2013-06-24 13:28:12 UTC
Created attachment 247644 [details] [review]
gobject: Add wrappers for overriding GProperty default values

We need convenience API for sub-classes that wish to override the
default value of a property installed by one of their parents.
Comment 106 Emmanuele Bassi (:ebassi) 2013-06-24 13:28:24 UTC
Created attachment 247645 [details] [review]
docs: Update examples in GProperty
Comment 107 Emmanuele Bassi (:ebassi) 2013-06-24 13:28:43 UTC
Created attachment 247646 [details] [review]
docs: Add a section on the GProperty macros
Comment 108 Emmanuele Bassi (:ebassi) 2013-06-24 13:28:54 UTC
Created attachment 247647 [details] [review]
docs: Update the GObject tutorial section on properties

Get rid of a bunch of Maman stuff, and use the new macros and functions
instead of the existing GParamSpec API.
Comment 109 Allison Karlitskaya (desrt) 2013-08-12 17:39:00 UTC
Talked about this on IRC, but writing it here as well.

After reviewing the latest branch, here are my main takeaways:

 - G_DEFINE_PROPERTIES as one top-level massive call is ... not nice

 - in general, the code generation macros are too complicated with their
   _BEGIN and _END separation.  We should have macros for the easy cases
   and allow the user to open-code the more complicated cases for themselves.

 - _INDIRECT_ should probably go.  We should always relay _set() via GProperty
   to get the ability to override and the nice automatic notify emissions, but
   we should always handle _get() directly for efficiency reasons.

 - the read/write/construct-only split should be read/write/construct instead
   where construct-only properties are indicated by having "construct" but not
   "write".  The ability to specify a writable property as being set during
   the construct phase is useful.

 - the call to g_property_canonicalize_name() and the for loop in the C setters
   needs to die.  I wonder if we can figure out a way to deal with this
   more efficiently than by iteration.  At the very least we should turn this
   into a once because it will find the same index every time.

   Another view: why are we finding the GProperty in the first place anyway?
   Couldn't we just go directly to the GParamSpecPool?  We're going to have
   to deal with the possibility of overrides anyway....

 - do we want to support non-private fields, as we did for templates?
Comment 110 Dan Winship 2013-11-05 23:36:21 UTC
Tried out the latest gproperty-2... lots of awesomeness

Comments in no particular order. Feel free to ignore any of them:

  - Putting the G_DEFINE_PROPERTY() calls into the G_DEFINE_PROPERTIES()
    call makes them get indented a bunch in a way that's annoying.
    (It doesn't matter if I hack my editor to not do that if everyone
    else hacking on my code doesn't do it too...) This perhaps ties into
    Ryan's complaint above, although in general, I like the
    simplifications that G_DEFINE_PROPERTIES() allows... Although:

  - You shouldn't have to specify the type name again in
    G_DEFINE_PROPERTY(). (Though I'm pretty sure there's absolutely
    nothing we can do about this.)

  - If you were to add flags G_PROPERTY_DISPOSE and G_PROPERTY_FINALIZE
    (meaning, "automatically set this property to NULL at dispose/finalize
    time"), then almost every class's dispose()/finalize() implementations
    could be simplified, and many of them could be removed entirely.

  - G_PROPERTY_COPY_SET needs to be the default behavior, with a flag for
    the opposite. There will be serious apocalypses if not.

  - This is kind of a weird idea, but... you could get rid of the need
    for separate G_DEFINE_PROPERTY_WITH_CODE() by making the "flags"
    argument of G_DEFINE_PROPERTY() actually be a "code" argument, and
    having the existing "flags" actually expand to something like

        #define G_PROPERTY_READABLE g_property_set_flag (g_property, G_PROPERTY_FLAG_READABLE);

    and then you would do:

        G_DEFINE_PROPERTY (MyObject, int, foobar,
                           G_PROPERTY_READWRITE
                           G_PROPERTY_CONSTRUCT_ONLY
                           G_PROPERTY_RANGE (0, 100)
                           G_PROPERTY_DEFAULT (12))

    This would also have the benefit of causing "value is ignored"
    warnings when you accidentally type "G_PARAM_READWRITE" instead of
    "G_PROPERTY_READWRITE", which I did roughly 90% of the time :)

  - I still don't like the use of the word "prerequisite"

  - g_property_boolean_set_value() ought to do the "value = !!value" thing

  - The property name is the most interesting argument to
    G_DEFINE_PROPERTY, G_DEFINE_PROPERTY_GET_SET, etc, so it feels weird
    that it comes so late in the argument list. (Counterargument:
    keeping the "constant" values (@T_N, @t_n, etc) at the front makes
    it easier to cut + paste + modify.)

  - It is a teensy weensy bit confusing that G_DEFINE_PROPERTY takes
    g-less pseudo types, but G_DEFINE_PROPERTY_GET_SET takes real types.
    Making G_DEFINE_PROPERTY() take all-caps type names might make it
    clearer that we're talking about GTypes there, not C types? (As I
    said, this isn't a huge thing, just something that briefly seemed
    weird to me the first time I ran into it.)

  - It seems un-GProperty-like to have to manually specify the complete
    getter/setter function names in G_DEFINE_PROPERTY_EXTENDED, though
    I'm not sure there's any fix for this that would actually make
    things better.

  - Using the GProperty macros in my class_init() makes the signal
    definitions look that much more ugly and archaic and I want
    G_DEFINE_SIGNALS now too :-)
Comment 111 Allison Karlitskaya (desrt) 2013-11-19 21:13:42 UTC
One more wishlist item while it's in my mind: we ought to make non-nullability the default here.
Comment 112 Emmanuele Bassi (:ebassi) 2013-12-17 11:18:35 UTC
vacation time, so I'm cutting up some time to get back working on this.

massive wall of text comment. I skipped some stuff that I don't have comments on...

(In reply to comment #109)
> Talked about this on IRC, but writing it here as well.
> 
> After reviewing the latest branch, here are my main takeaways:
> 
>  - G_DEFINE_PROPERTIES as one top-level massive call is ... not nice

after working with it, yes: it isn't. the main reason why it's there it's because of the static GProperty array that I use for storing the properties for look up to bypass the GParamSpecPool, and to install in one go instead of having to call g_object_class_install_property() for every property. if I change G_DEFINE_TYPE_EXTENDED to set up the GParamSpec array then I can get rid of the massive macro, and just have a G_DEFINE_PROPERTY macro instead.

>  - the read/write/construct-only split should be read/write/construct instead
>    where construct-only properties are indicated by having "construct" but not
>    "write".  The ability to specify a writable property as being set during
>    the construct phase is useful.

I feel like the whole construct/construct-only issue needs to be discussed separately. I've heard 10 different opinions from 9 different people, at this point. we should have a clear set of rules for what kind of annotation we want.

a summary could be:

  * properties annotated as 'construct' may be passed to the constructor
    * unless if they are marked as 'required' in which case the constructor will warn if it does not see them
  * 'construct' properties without the 'writable' bit set cannot be written after the constructor returns (i.e. construct-only)

which I think matches your intent, and the request from the Python bindings community to have a way to mark required constructor properties (there's at least a bug opened about it).

>  - the call to g_property_canonicalize_name() and the for loop in the C setters
>    needs to die.  I wonder if we can figure out a way to deal with this
>    more efficiently than by iteration.  At the very least we should turn this
>    into a once because it will find the same index every time.

yeah, that's clearly unoptimised crap. what I'm really trying to avoid:

  * getting the class pointer from the instance
  * calling g_object_class_find_property()
    * traversing the GParamSpecPool
    * and taking *all* the locks

>    Another view: why are we finding the GProperty in the first place anyway?
>    Couldn't we just go directly to the GParamSpecPool?  We're going to have
>    to deal with the possibility of overrides anyway....

getting the GParamSpecPool could be feasible, but if we're trying to get rid of it I'd rather avoid exposing it into the code generation macros. if we're going to hide it into a function then we might as well call g_object_class_find_property() — but see above, re: accessing the GParamSpecPool.

given that the need to access the GParamSpec for g_object_notify() is vastly reduced by GProperty maybe we should just bite the bullet, but it's still sucks.

maybe just do a g_object_class_find_property() inside a GOnce would be enough.

>  - do we want to support non-private fields, as we did for templates?

for the code generation macros? we can. the API supports both, so in theory you could just call g_*_property_new() with G_STRUCT_OFFSET yourself. the naming of the macro is a bit weird: to me, G_DEFINE_PUBLIC_PROPERTY_* or G_DEFINE_PRIVATE_PROPERTY_* seem to relate to the visibility of the property, not to the location of the storage for that property.

(In reply to comment #110)
>   - If you were to add flags G_PROPERTY_DISPOSE and G_PROPERTY_FINALIZE
>     (meaning, "automatically set this property to NULL at dispose/finalize
>     time"), then almost every class's dispose()/finalize() implementations
>     could be simplified, and many of them could be removed entirely.

I don't think we need a separate flag for that: we can automatically clear all pointer types, and free/unref them if the COPY_SET flag is set, as we know that the value is going to be a copy.

>   - G_PROPERTY_COPY_SET needs to be the default behavior, with a flag for
>     the opposite. There will be serious apocalypses if not.

COPY_SET could stay a flag that is set automatically for all pointer types, and we can have a:

  #define G_PROPERTY_FLAGS_NO_COPY_SET ~(G_PROPERTY_FLAGS_COPY_SET)

that you pass if you want to store a pointer.

>   - This is kind of a weird idea, but... you could get rid of the need
>     for separate G_DEFINE_PROPERTY_WITH_CODE() by making the "flags"
>     argument of G_DEFINE_PROPERTY() actually be a "code" argument, and
>     having the existing "flags" actually expand to something like
> 
>         #define G_PROPERTY_READABLE g_property_set_flag (g_property,
> G_PROPERTY_FLAG_READABLE);
> 
>     and then you would do:
> 
>         G_DEFINE_PROPERTY (MyObject, int, foobar,
>                            G_PROPERTY_READWRITE
>                            G_PROPERTY_CONSTRUCT_ONLY
>                            G_PROPERTY_RANGE (0, 100)
>                            G_PROPERTY_DEFAULT (12))
> 
>     This would also have the benefit of causing "value is ignored"
>     warnings when you accidentally type "G_PARAM_READWRITE" instead of
>     "G_PROPERTY_READWRITE", which I did roughly 90% of the time :)

I actually like this idea.

>   - I still don't like the use of the word "prerequisite"

I gladly accept different names.

>   - The property name is the most interesting argument to
>     G_DEFINE_PROPERTY, G_DEFINE_PROPERTY_GET_SET, etc, so it feels weird
>     that it comes so late in the argument list. (Counterargument:
>     keeping the "constant" values (@T_N, @t_n, etc) at the front makes
>     it easier to cut + paste + modify.)

copy and paste was one of the two main reasons for the arguments list being the way it is, the other being: it matches the way you declare variables, arguments, and fields in C:

  TypeName field_name

so the macro reads:

  define a property for class ClassName of type TypeName with name field_name

>   - It is a teensy weensy bit confusing that G_DEFINE_PROPERTY takes
>     g-less pseudo types, but G_DEFINE_PROPERTY_GET_SET takes real types.
>     Making G_DEFINE_PROPERTY() take all-caps type names might make it
>     clearer that we're talking about GTypes there, not C types? (As I
>     said, this isn't a huge thing, just something that briefly seemed
>     weird to me the first time I ran into it.)

no, it's not GTypes. I mean, yes: underneath it all there's a GType, but the overall idea is to be closer to the C type you use for storing the value. GType is not expressive enough, especially when it comes to integers. also, a good deal of g* typedefs are pretty stupid, except when it comes to sized integers.

using all caps names would make g_*_property_new() look like:

  g_BOOLEAN_property_new()

which would look pretty dumb.

maybe we could use g_gboolean_property_new(), g_gint_propert_new(), ... and so on and so forth; then the G_DEFINE_PROPERTY() would use the g* typedef name instead of the g* typedef minus the 'g' part.

>   - It seems un-GProperty-like to have to manually specify the complete
>     getter/setter function names in G_DEFINE_PROPERTY_EXTENDED, though
>     I'm not sure there's any fix for this that would actually make
>     things better.

those are internal functions, so they can have whatever name you want.

>   - Using the GProperty macros in my class_init() makes the signal
>     definitions look that much more ugly and archaic and I want
>     G_DEFINE_SIGNALS now too :-)

one step at a time, I have plans for that too. ;-)

(In reply to comment #111)
> One more wishlist item while it's in my mind: we ought to make non-nullability
> the default here.

we can implement non-nullability as a flag like we do the COPY_SET. add a G_PROPERTY_FLAGS_NON_NULLABLE, set it by default on pointer types, and provide a #define for NULLABLE.
Comment 113 Dan Winship 2013-12-17 13:55:45 UTC
(In reply to comment #112)
> >   - If you were to add flags G_PROPERTY_DISPOSE and G_PROPERTY_FINALIZE
> >     (meaning, "automatically set this property to NULL at dispose/finalize
> >     time"), then almost every class's dispose()/finalize() implementations
> >     could be simplified, and many of them could be removed entirely.
> 
> I don't think we need a separate flag for that: we can automatically clear all
> pointer types, and free/unref them if the COPY_SET flag is set, as we know that
> the value is going to be a copy.

But do you do that at dispose() time or at finalize() time?

> using all caps names would make g_*_property_new() look like:
> 
>   g_BOOLEAN_property_new()
> 
> which would look pretty dumb.

yes...

you could get around that by having a const static array of the new() funcs and then have the macro expand to "g_property_new_funcs[G_TYPE_ ## type] ()" rather than "g_ ## type ## _property_new ()"...

Except that as you said, some of the choices aren't actual GTypes, so that wouldn't really work I guess.

> maybe we could use g_gboolean_property_new(), g_gint_property_new()

Currently GSimpleAsyncResult is the only public API in glib that uses g-prefixed fundamental type names in function names, and it's deprecated, so I'd vote against this for the sake of consistency.
Comment 114 Emmanuele Bassi (:ebassi) 2013-12-17 16:40:01 UTC
(In reply to comment #113)
> (In reply to comment #112)
> > >   - If you were to add flags G_PROPERTY_DISPOSE and G_PROPERTY_FINALIZE
> > >     (meaning, "automatically set this property to NULL at dispose/finalize
> > >     time"), then almost every class's dispose()/finalize() implementations
> > >     could be simplified, and many of them could be removed entirely.
> > 
> > I don't think we need a separate flag for that: we can automatically clear all
> > pointer types, and free/unref them if the COPY_SET flag is set, as we know that
> > the value is going to be a copy.
> 
> But do you do that at dispose() time or at finalize() time?

I split the boxed/string free at finalize() and the object unref() at dispose().

I'm tending towards the automatic handling because having two more property flags/annotations tied to the GObject dispose/finalize cycle may be confusing (I can already see the questions on stackoverflow: "what should I use for a string?"); also because we ought to have all the information and annotation for this kind of stuff.

> > using all caps names would make g_*_property_new() look like:
> > 
> >   g_BOOLEAN_property_new()
> > 
> > which would look pretty dumb.
> 
> yes...
> 
> you could get around that by having a const static array of the new() funcs and
> then have the macro expand to "g_property_new_funcs[G_TYPE_ ## type] ()" rather
> than "g_ ## type ## _property_new ()"...
> 
> Except that as you said, some of the choices aren't actual GTypes, so that
> wouldn't really work I guess.
> 
> > maybe we could use g_gboolean_property_new(), g_gint_property_new()
> 
> Currently GSimpleAsyncResult is the only public API in glib that uses
> g-prefixed fundamental type names in function names, and it's deprecated, so
> I'd vote against this for the sake of consistency.

yeah, that's pretty much the API I was thinking about.

there's an additional asymmetry with regards to strings: autogenerated accessors use const char* as the type, but the field in the struct is char*, and the type used when declaring the property is "string", which is what we use everywhere else - GParamSpec, GValue, etc.

dunno, I think we'll have to live with the slight inconsistency, unless we introduce a GCString refcounted string type and move all our code to using it.
Comment 115 Bastien Nocera 2014-04-15 17:02:26 UTC
GProperty would very useful for users of gom (see the gom repo in GNOME git). Objects (that translate to rows) are subclasses of GomResource, and are just property bags.
Comment 116 GNOME Infrastructure Team 2018-05-24 13:08:05 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/408.