GNOME Bugzilla – Bug 760109
[PATCH] Invalid GDate can't be g_boxed_copy()'d
Last modified: 2017-10-11 11:17:20 UTC
Currently, if you want to put a GDate in a GObject, the date must be set to some valid date. If the GObject is constructed and the property is simply g_date_clear()'d, it is impossible to g_object_get() the GDate unless it has been set to some valid value. This is inconvenient; gdate_copy() should always provide a copy, whether or not the date is invalid. I have a patch that adds a g_date_copy() method that gboxed.c can call instead of the current gdate_copy() function.
Created attachment 318191 [details] [review] Add g_date_copy() method to copy even invalid GDates
Created attachment 318192 [details] [review] Add g_date_copy() method to copy even invalid GDates
Review of attachment 318192 [details] [review]: ::: glib/gdate.c @@ +349,3 @@ + * Copies a GDate to a newly-allocated GDate. If the input was invalid + * (as determined by g_date_valid()), the invalid state will be copied + * verbatum into the new object. It's "verbatim", not "verbatum". I'd use "as is", instead. To be fair, I'm not entirely sure it's a good idea to do this. I literally don't see the point, especially for the state objective of being able to get a GDate from a GObject property. I'd either return NULL if the date is invalid, or I'd return a new, unset GDate instance. @@ +351,3 @@ + * verbatum into the new object. + * + * Returns: a newly-allocated #GDate initialized from @date Missing a `Since: 2.48` annotation, and the method needs to be added to the glib-sections.txt file in order to be picked up by gtk-doc. @@ +364,3 @@ + { + res = g_date_new (); +GDate* You should do: *res = *date; to allow the compiler to optimize the mem copy.
Created attachment 318196 [details] [review] Add g_date_copy() method to copy even invalid GDates Addressed your comments, thanks. Regarding how necessary it is, you can't return NULL with the current implementation. foo_get_property(GObject *object, guint property_id, GValue *value, GParamSpec *pspec) { Foo *self = FOO(object); FooPrivate *priv = foo_get_instance_private(self); static GDate date_zero; g_date_clear(&date_zero, 1); switch (property_id) { case PROP_DATE: if (g_date_valid(&priv->date)) { g_value_set_boxed(value, &priv->date); } else { g_value_set_boxed(value, &priv->date); /* No good, g_value_set_boxed() will call g_boxed_copy()->gdate_copy()->g_date_get_julian() and g_critical() from g_return_val_if_fail */ g_value_set_static_boxed(value, &date_zero); /* No good, boxed_proxy_lcopy_value() will still call gdate_copy()->g_date_get_julian() and then g_critical() */ g_value_set_boxed(value, NULL); /* No good, boxed_proxy_lcopy_value() will still call gdate_copy()->g_date_get_julean() and then g_critical() */ } }
(In reply to Andrew Potter from comment #4) > g_value_set_boxed(value, NULL); /* No good, > boxed_proxy_lcopy_value() will still call gdate_copy()->g_date_get_julean() > and then g_critical() */ Sigh, my bad. My test case was failing on a different field when I tried this. It does work, but being forced to return a NULL is less than ideal. Trying to use g_date_new() and then g_value_take_boxed() does not work. A test case is in this gist: https://gist.github.com/talisein/a2245d0f1ec0c89df190
Created attachment 342641 [details] [review] gdate: add g_date_copy() Updated patchset to say available in 2.52 instead of 2.48. It would be nice to get this simple patch in before its first annual birthday.
Review of attachment 342641 [details] [review]: There are a few nitpicks with the code formatting, and it now needs to be Since: 2.56, but I’ll fix those issues up myself when I push it. Thanks for the patch.
Attachment 342641 [details] pushed as 5564dde - gdate: add g_date_copy()