Bug 77197

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.

(In reply to comment #1)
> +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.

Right, it's much better to have transfer full for both the arg and the return. Changed it in a fixup commit.

> -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.

Not sure to understand why it wouldn't be introspectable. The binding will just deep-copy the whole thing, no? Elements are boxed type so it knows how to copy.

I changed to return a GPtrArray in a fixup commit, it's nicer IMO.

> + * TpProtocolAddressing:
> + *
> + * <!-- -->
> + *
> + * Since: 0.UNRELEASED
> + */
> 
> Either put in the STANDARD section, or write some actual text. I'd go for
> the former.

Indeed.

> + * @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.

Changed to take an user_data and destroy extra args and use scope notified. This is actually the reason I refcounted the struct otherwise we would need a way to copy user_data.

Wow, this is a mega-branch. I've taken the first 5 non-fixup commits plus their fixups, and am still working on making Gabble pass tests against them, let alone the other CMs...

(In reply to comment #3)
> Wow, this is a mega-branch. I've taken the first 5 non-fixup commits plus
> their fixups, and am still working on making Gabble pass tests against them,
> let alone the other CMs...

I gave up on that, and went for "first 2 non-fixup" as Bug #77194.

I really do think that letting telepathy-glib run way ahead of the CMs etc. is a bad idea - we end up with changes in telepathy-glib that look vaguely reasonable, but do not, in fact, work, because for hysterical raisins a lot of our test coverage is actually in the CMs.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/128.