[Bug 77197] [next] TpBaseProtocol, TpCMParamSpec, etc.: be GVariant-based

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=77197

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Reviewing 1460a071 from xclaesse's gdbus-all branch:

This commit is pretty noisy. It might have been better a bit more broken up,
but perhaps life's too short.

Please excuse the wrong order, I'm reviewing backwards so I get to the examples
after reading the implementation :-)

+typedef GVariant *(*TpCMParamFilter) (const TpCMParamSpec *paramspec,
+    GVariant *value, GError **error);

I see this returns (transfer none)? I wonder whether it should be (transfer
full) - otherwise it's approximately impossible to implement (pseudocode)
"return value.lower()" in introspected languages without a leak. (It would be
possible in C, but only by returning a floating ref.)

I suspect it would be better to use the same signature as
GDBusMessageFilterFunction - be given ownership of a ref, and return a ref
(meaning pass-through is easy, and altering or dropping is not difficult).

In introspection-land, in practice this filter might well return a new variant
whose type and value happen to match the input, rather than the exact same
variant it was given.

-const TpCMParamSpec *
+const TpCMParamSpec * const *
 tp_base_protocol_get_parameters (TpBaseProtocol *self)

I find it hard to imagine how introspected code could be expected to get this
right, except by leaking it: in introspected code, the internal data structure
is not going to be a GPtrArray<TpCMParamSpec *> or anything like that, it's
going to be something like a Python list of Python objects that each wrap a
TpCMParamSpec. The "least copied" thing we can expect introspected code to get
right is (transfer container).

I would go for a (transfer container) GPtrArray (although I'm not sure how well
that would introspect either...) or a (transfer full) GList. If using the
latter, TpCMParamSpec should become refcounted so that it remains cheap.

+ * TpProtocolAddressing:
+ *
+ * <!-- -->
+ *
+ * Since: 0.UNRELEASED
+ */

Either put in the STANDARD section, or write some actual text. I'd go for the
former.

+ * @filter: (allow-none) (scope call): A filter function to validate ...
+TpCMParamSpec *
+tp_cm_param_spec_new (const gchar *name,
+    TpConnMgrParamFlags flags,
+    GVariant *def,
+    TpCMParamFilter filter)

(scope call) is clearly untrue: it can be called after tp_cm_param_spec_new()
returns.

The filter should come with user data, and a destructor for the user data, so
it can be (scope notify) or whatever it's called.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.