[Bug 683012] check: add GstTestClock for use in unit testing

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=683012
  GStreamer | gstreamer (core) | git

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #223057|none                        |reviewed
             status|                            |

--- Comment #5 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2012-11-13 21:10:42 UTC ---
Review of attachment 223057:
 --> (https://bugzilla.gnome.org/review?bug=683012&attachment=223057)

Looks great! Only some minor niggles, but generally good to go IMHO.

::: libs/gst/check/gsttestclock.c
@@ +60,3 @@
+ * arguments. This will highlight any issues with the unit test code itself.
+ */
+

Should still include config.h first thing, with HAVE_CONFIG_H guards.

@@ +66,3 @@
+{
+  PROP_0,
+  PROP_START_TIME,

No trailing comma please, some compilers really don't like that (yes, really).

@@ +83,3 @@
+};
+
+#define GST_TEST_CLOCK_GET_PRIVATE(obj) ((GST_TEST_CLOCK_CAST (obj))->priv)

I would prefer to drop this macro and just do testclock->priv->foo directly.
Easier to read, fewer variables, more in line with how it's used elsewhere. In
a few performance-sensitive cases it might make sense to retrieve FooPrivate
first and then use priv->foo.

@@ +179,3 @@
+{
+  G_OBJECT_CLASS (parent_class)->finalize (object);
+}

Why override and implement dispose and finalize if we don't have anything to
do?

@@ +186,3 @@
+{
+  GstTestClock *test_clock = GST_TEST_CLOCK (object);
+  GstTestClockPrivate *priv = GST_TEST_CLOCK_GET_PRIVATE (test_clock);

See above about test_clock->priv->foo

@@ +194,3 @@
+    default:
+      G_OBJECT_CLASS (parent_class)->set_property (object, property_id, value,
+          pspec);

This is not right. {get,set}_property are special vfuncs. GObject maps the
property IDs to the right class, so there's no need to "chain up" to the parent
class. The default label should contain the standard warning about an
unrecognises property_id, which you can copy'n'paste from elsewhere (not that
that code is likely to ever be called..).

@@ +287,3 @@
+ * gst_clock_get_time() is a programming error.
+ *
+ * MT safe.

Please add a "Since: 1.2" marker, to the other doc chunks as well.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.