Re: [Vala] Issues will vala and pulse vapi

[Evan Nemerson <evan coeus-group com>]

On Mon, 2015-11-23 at 00:56 +0000, Al Thomas wrote:
> > From: Evan Nemerson <evan coeus-group com>
>
> > Sent: Sunday, 22 November 2015, 3:54
> > Subject: Re: [Vala] Issues will vala and pulse vapi
> >
> > On Sat, 2015-11-21 at 20:48 -0600, Aaron Paden wrote:
> > >  Right, the code compiles, but the linker fails because of the
> > >  unresolved symbols. I didn't think of this before, but the
> > > minimal
> > >  example is pretty simple:
> > >
> > >  void main() {
> > >    PulseAudio.SourceInfo info;
> > >  }
> > If you don't provide a destroy_function CCode annotation but Vala
> > determines that it is necessary to call one, it will default to
> > lower_case_cprefix_destroy, which is why it is generating a call to
> > pulse_audio_source_info_destroy.
>
>
>
> Hmm, just found this in the Legacy Bindings documentation (
> https://wiki.gnome.org/Projects/Vala/LegacyBindings#Structs ):
>
> "It's important to note that if a struct doesn't have a destroy
> function specified, Vala will generate one given there are any fields
> in the struct which look like they need to be deallocated, which may
> or may not behave correctly depending on the context. An empty
> destroy_function will keep the generated code correct and prevent
> Vala from generating a destructor."
>
> So in the VAPI try:
> [CCode (cname="pa_sink_info", destroy_function = "",
> has_type_id=false)]
> public struct SinkInfo {
> //...
> }

That's almost always a spectacularly bad idea.  For example, if you do
that then:

    private void foo () {
      SinkInfo bar;
      info.name = "baz";
    }

When assigning "baz" to info.name the string will get duped (g_strdup)
since you're assigning to an owned field.  However, it never gets
freed.

This can happen in more subtle ways, too.  For example

    private void callback (SinkInfo info) {
      // For some reason we want to change the name
      info.name = "baz";
    }

Will cause the old value to be leaked and, if the caller (i.e., PA)
doesn't free the field the new one will be leaked as well.

If the field should really be unowned then right thing to do is fix the
binding to make the field unowned.  If the field should be owned (think
about the second example; this is important if PA is freeing those
fields at some point) and it's marked as unowned in the bindings you're
likely to end up with a double-free.

The only time any of this should really be an issue is where fields are
owned but the library doesn't expose a destroy function, which I would
consider to be a bug in the library.  In that case, the right solution
would be to add a has_destroy_function = false CCode annotation.

I'm having trouble coming up with an example where destroy_function =
"" is actually a good idea.

Also, it will generate a call to "".  In other words, you'll have
something like this in your generated C:

    info.name = g_strdup ("baz");
     (&info);

Which, while valid C (AFAIK, though a CC is likely to emit a warning
about a statement with no effect), is more horrible than even the tmp
variable madness valac usually generates.  Maybe not a bug, but
illustrative of just how much of a hack this is.  OTOH,
has_destroy_function = false will generate a destroy function which
will call the relevant destroy/free functions for each field.

Basically, the only real use case I can think of for destroy_function =
"" is when you *want* the field to leak, and there are other ways to
accomplish that.


-Evan