Code review comment for lp://staging/~ted/dbus-test-runner/dbus-mock

On Mon, 2013-11-18 at 17:22 +0000, Charles Kerr wrote:

> * Why is DbusTestProcess.parameters a GArray instead of a GPtrArray?

Because I wanted it to be NULL terminated, which GPtrArray doesn't do.
That makes data usable as a GStrv.

> * Why are DbusTestDbusMock.properties and .methods GArrays instead of GPtrArrays?

To avoid allocating the structures separately. Basically all memory
management moves with the array then.

> * in dbus-mock.c's install_object(), do the local "properties" and "methods"
> GVariants need to be unreffed, or does ..._add_object_sync() always consume them,
> even on failure?

Yeah, it creates another, larger variant with g_variant_new().

> * in dbus-mock.c's dbus_test_dbus_mock_object_update_property(), maybe use
> g_propagate_error() instead of direct assignmnt "*error = local_error"?

Ah, always forget about that function. Fixed r160.

> * in test-libdbustest-mock.c's main(), should G_LOG_LEVEL_CRITICAL be fatal
> in the tests?

No, because one of the tests purposefully make a critical error to make
sure it's handled:

libdbustest-CRITICAL **: Property 'prop1' is not of same value in
dbus_test_dbus_mock_object_update_property()

> * in process.c's dbus_test_process_get_pid(), need to use g_return_val_if_fail()
> instead of g_return_if_fail()

Fixed r161.

> * in dbus-mock.c's dbus_test_mock_dispose(), need to test self->priv->cancel for
> NULL before passing it to g_cancellable_cancel()

Fixed r162.

> * in dbus-mock.c's method_params_to_variant(), please add a comment above the
> "if (params == NULL)" explaining what DBusMock is expecting when this happens?
> I don't understand this block.

Explained r163.

> * in dbus-mock.c's method_params_to_variant(), need to test len>0 before using
> peek[0]. Should probably also test peek!=NULL before using strlen().

Fixed r164.

> * in dbus-mock.c's install_object(), dead assign of NULL to the local "properties"
> and "method" GVariants when declaring them. Better to leave them uninitialized so
> gcc can yell at tell us if they get used without being initialized later in
> the code.

Hmm, I think this is done already. Do you see it in another place in
the code?

> * in process.c's dbus_test_process_append_param(), ignore the return value of
> g_array_append_val(). This is a leftover from switching from GList -> GArray

Wow. Fixed r165.

> * in dbus-mock.c's constructed(), multiple commands on single lines.
> Might be more readable to remove the paramval variable altogether and use
> g_array_append_val (params, "-m");
> g_array_append_val (params, "dbusmock");

Sadly, you can't. The macro requires it to be a local variable. I did
that to make it seem a little less insane, not sure if I accomplished
that or not.

> * in test-lidbustest-mock.c, inconsistent use of "static" prefix for functions:
> signal_emitted() is private, but timeout_quit_func(), process_mainloop(), etc
> are public

Fixed r166.

> * in test-libdbustest-mock.c, lots of leaked "testvar"s in multiple functions

Actually it was the tuple getting leaked :-) Fixed r167.