GNOME Bugzilla – Bug 536721
Added Glib::Date GDate constructor.
Last modified: 2008-07-11 16:04:43 UTC
While working on some functions in gstreamermm that deal with Glib::Date, I noticed the need for a Glib::Date GDate constructor (it is actually a TODO). I noticed that it was in the source file, but I made it visible and modified it to use a GDate pointer and not a GDate reference. I also added a copy constructor and an assignment operator and modified the gobject_ variable to be a GDate pointer instead of a GDate reference (adding a destructor to free the gobject_). This makes the GDate constructor more logical by freeing the GDate* when the Glib::Date is destroyed. I'm not sure if there are any binary compatibility issues, but I think these changes will make Glib::Date more usable. Is the included patch ok?
Created attachment 112181 [details] [review] Glib::Date GDate constructor patch
Created attachment 112455 [details] [review] Replacement patch to add test (and make gobject_ private) I modified the patch because I had the member variable "gobject_" protected and not private. This is what I really wasn't sure about because I'm not sure if variable should remain protected and destructor virtual so class could be extended or if variable should simply be private. For now I made variable private and added a test.
+private: + GDate *gobject_; -private: - GDate gobject_; This would break ABI by changing the size of the whole class instance. Can't the contents of the GDate be copied?
(In reply to comment #3) > +private: > + GDate *gobject_; > > -private: > - GDate gobject_; > > This would break ABI by changing the size of the whole class instance. Can't > the contents of the GDate be copied? > In a way, I kind of thought it would but wasn't entirely sure. Would it also be broken if the signature of the GDate constructor be changed from `Date(const GDate&)' to `Date(const GDate*)'?
> Would it also be broken if the signature of the GDate constructor be changed > from `Date(const GDate&)' to `Date(const GDate*)'? But you said that neither of those constructors exists yet, so that wouldn't be a break.
(In reply to comment #5) > > Would it also be broken if the signature of the GDate constructor be changed > > from `Date(const GDate&)' to `Date(const GDate*)'? > > But you said that neither of those constructors exists yet, so that wouldn't be > a break. > Did I? Sorry. `Date(const GDate&)' does exist; it's hidden by a `#ifndef DOXYGEN_SHOULD_SKIP_THIS' block at the moment.
Created attachment 112491 [details] [review] Patch to make GDate constructor visible (adds copy constructor and assignment operator) This patch does not modify the signatures, access rights or types of members or methods. It makes the GDate constructor visible and adds a copy and assignment constructor. It also adds a simple test.
(In reply to comment #6) > (In reply to comment #5) > > > Would it also be broken if the signature of the GDate constructor be changed > > > from `Date(const GDate&)' to `Date(const GDate*)'? > > > > But you said that neither of those constructors exists yet, so that wouldn't be > > a break. > > > > Did I? Sorry. `Date(const GDate&)' does exist; it's hidden by a `#ifndef > DOXYGEN_SHOULD_SKIP_THIS' block at the moment. > It was the bug report subject that implied that there was no GDate constructor. Once more, sorry about that.
That looks good. Please commit.