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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Xavier Claessens <xclaesse at gmail.com> ---
(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.

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