GNOME Bugzilla – Bug 770401
docs: Add type checking for GObject properties in tutorial
Last modified: 2018-05-24 19:03:38 UTC
Trivial patch attached.
Created attachment 334157 [details] [review] docs: Add type checking for GObject properties in tutorial This improves the behaviour of the code snippets with -Wswitch-enum to enable more compile-time checks for property names.
Review of attachment 334157 [details] [review]: ::: docs/reference/gobject/tut_gobject.xml @@ +470,3 @@ PROP_FILENAME = 1, PROP_ZOOM_LEVEL, + /*< private >*/ No need. ::: docs/reference/gobject/tut_howto.xml @@ +340,3 @@ PROP_FILENAME = 1, PROP_ZOOM_LEVEL, + /*< private >*/ No need for this. @@ +341,3 @@ PROP_ZOOM_LEVEL, + /*< private >*/ + LAST_PROP = PROP_ZOOM_LEVEL, Why the rename? N_PROPERTIES is perfectly cromulent, and it's more appropriate to read when declaring the obj_properties GParamSpec array. @@ +1248,3 @@ PROP_AUTOSAVE_FREQUENCY = 1, + /*< private >*/ + LAST_PROP = PROP_AUTOSAVE_FREQUENCY, Same as above. @@ +1259,3 @@ ViewerFile *file = VIEWER_FILE (object); + switch ((ViewerFileProperty) prop_id) Still think it kinda sucks. BTW, if we're using an explicit type for property ids and compiler warnings, then we should drop the: default: G_OBJECT_WARN_INVALID_PROPERTY_ID (gobject, prop_id, pspec); fragment. @@ +1279,3 @@ ViewerFile *file = VIEWER_FILE (object); + switch ((ViewerFileProperty) prop_id) Same as above. ::: gobject/gobject.c @@ +613,3 @@ + * PROP_FOO = 1, + * PROP_BAR, + * /<!-- -->*< private >*<!-- -->/ This is pointless: the whole typedef is private to the implementation. The only reason why we use the "/*< private >*/" annotation is for public enumeration types that are annotated by gtk-doc stanzas. @@ +1229,3 @@ * { + * PROP_FOO = 1, + * /<!-- -->*< private >*<!-- -->/ Same as above.
(In reply to Emmanuele Bassi (:ebassi) from comment #2) > Review of attachment 334157 [details] [review] [review]: > > ::: docs/reference/gobject/tut_gobject.xml > @@ +470,3 @@ > PROP_FILENAME = 1, > PROP_ZOOM_LEVEL, > + /*< private >*/ > > No need. True. > @@ +341,3 @@ > PROP_ZOOM_LEVEL, > + /*< private >*/ > + LAST_PROP = PROP_ZOOM_LEVEL, > > Why the rename? N_PROPERTIES is perfectly cromulent, and it's more > appropriate to read when declaring the obj_properties GParamSpec array. LAST_PROP is not the same as N_PROPERTIES; it’s N_PROPERTIES-1. This is necessary to avoid a warning from -Wswitch-enums about not handling N_PROPERTIES in the switch in get_property()/set_property(). gcc handily avoids emitting that warning for enum members which alias another. > @@ +1259,3 @@ > ViewerFile *file = VIEWER_FILE (object); > > + switch ((ViewerFileProperty) prop_id) > > Still think it kinda sucks. What? This is, like, the least sucky bit of the patch. > BTW, if we're using an explicit type for property ids and compiler warnings, > then we should drop the: > > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID (gobject, prop_id, pspec); > > fragment. I kind of agree, but that then falls foul of -Wswitch-default.
Ping?
-- 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/1195.