'out unowned Graphene.Matrix res' corresponds to 'graphene_matrix_t **res' when Graphene.Matrix is a class, which doesn't match the C API, i.e., the binding is incorrect.

graphene_matrix_t should be bound as a Vala struct, not class. It can be stack-allocated and freely copied by value, as far as I can tell.

Turns out, this is from vapigen producing code that is mismatched from the C. This is either a bug in vapigen or the source GIRs.

I'm pretty sure this is a bug in vapigen with caller-allocated out parameters.

For example:

This declaration:

void                    graphene_matrix_multiply                (const graphene_matrix_t  *a,
                                                                 const graphene_matrix_t  *b,
                                                                 graphene_matrix_t        *res);


With this gir:

      <method name="multiply"
              c:identifier="graphene_matrix_multiply"
              version="1.0">
        <doc xml:space="preserve">Multiplies two #graphene_matrix_t.

Remember: matrix multiplication is not commutative, except for the
identity matrix; the product of this multiplication is (A * B).</doc>
        <return-value transfer-ownership="none">
          <type name="none" c:type="void"/>
        </return-value>
        <parameters>
          <instance-parameter name="a" transfer-ownership="none">
            <doc xml:space="preserve">a #graphene_matrix_t</doc>
            <type name="Matrix" c:type="const graphene_matrix_t*"/>
          </instance-parameter>
          <parameter name="b" transfer-ownership="none">
            <doc xml:space="preserve">a #graphene_matrix_t</doc>
            <type name="Matrix" c:type="const graphene_matrix_t*"/>
          </parameter>
          <parameter name="res"
                     direction="out"
                     caller-allocates="1"
                     transfer-ownership="none">
            <doc xml:space="preserve">return location for the matrix
  result</doc>
            <type name="Matrix" c:type="graphene_matrix_t*"/>
          </parameter>
        </parameters>
      </method>

Produced this VAPI:

                [Version (since = "1.0")]
                public void multiply (Graphene.Matrix b, out unowned Graphene.Matrix res);

"out unowned" causes valac to produce a second indirection (ie, **), which is incorrect.

al on #vala points to https://git.gnome.org/browse/vala/tree/vala/valagirparser.vala#n2376 as lacking caller-allocates support.

The main issue is not the method but the type. GIR might not be sufficiently expressive such that Vala can recognize that the type should be bound as Vala struct instead of class. Metadata
        Matrix struct
should help.

Jürg, 

As you say, Matrix is a struct. From what I can see, however, the struct is passed by reference. From https://github.com/ebassi/graphene/blob/master/src/graphene-matrix.h:

void graphene_matrix_multiply const graphene_matrix_t *a, const graphene_matrix_t *b, graphene_matrix_t *res);

There are a number of functions that act upon this struct, translate, rotate_x, etc., that suggest a compact class is a good way of binding it. Other functions take the struct as a pointer to a constant, such as multiply. Having those bound as a compact class shouldn't be a problem. All that is being passed is the pointer to the struct - the instance data. I can see the GObject binding uses box type for GrapheneMatrix: https://github.com/ebassi/graphene/blob/master/src/graphene-gobject.c Is that a problem?

As you say "'out unowned Graphene.Matrix res' corresponds to 'graphene_matrix_t **res' when Graphene.Matrix is a class, which doesn't match the C API, i.e., the binding is incorrect." What we've found is 'ref Graphene.Matrix res' corresponds to 'graphene_matrix_t *res' wehn Graphene.Matrix is a class and this is the C API. I'm not sure what I'm missing here.

In the discussion on IRC this patch for gobject-introspection was found: https://bugzilla.gnome.org/show_bug.cgi?id=604749
This creates two type of "out" parameter in the GIR file. The first is:

<parameter name="res"
  direction="out"
  caller-allocates="0"

and the second is 

<parameter name="res"
  direction="out"
  caller-allocates="1"

The first maps to "out" in Vala, the second best maps to "ref". The "transfer-ownership" attribute in the GIR also needs to be taken account of.

At present the caller-allocates attribute is ignored by the Vala GIR parser. Is that how it should be?

Methods are also available for structs. And Vala structs are passed by reference on the C level by default. graphene_matrix_t appears to typically be stack-allocated (or as field of larger struct). This is only possible for structs in Vala. Why do you consider a compact class to be a more suitable binding? cairo_matrix_t is a very similar type and is bound as Vala struct.

(In reply to Al Thomas from comment #6)
> As you say "'out unowned Graphene.Matrix res' corresponds to
> 'graphene_matrix_t **res' when Graphene.Matrix is a class, which doesn't
> match the C API, i.e., the binding is incorrect." What we've found is 'ref
> Graphene.Matrix res' corresponds to 'graphene_matrix_t *res' wehn
> Graphene.Matrix is a class and this is the C API. I'm not sure what I'm
> missing here.

No, that's not correct. 'ref' and 'out' parameters always use the same C type. They only differ in initialization requirements. 'ref' parameters are in/out parameters, which means that the caller has to initialize the argument before the call and the callee can but doesn't have to change the value. 'out' parameters must be initialized by the callee, i.e., 'out' parameters are semantically like extra return values.

(In reply to Jürg Billeter from comment #7)
> Why do you consider a compact class to be a more suitable binding?

For the following reasons:

 - It appeared to be heap allocated because graphene_matrix_alloc () and graphene_matrix_free () exist. Is that the distinguishing feature of a compact class - it can only be heap allocated? and conversely a struct in a binding can only be stack allocated, although there can be fields that are heap allocated?

 - The docs for graphene_matrix_t state "The contents of the graphene_matrix_t structure are private and should never be accessed directly." I interpreted this as an opaque structure and the Vala Legacy Bindings doc state "The full structure for foo must be given for a struct. For compact classes it may be opaque (i.e., the contents of the struct are not provided), though not necessarily. "

 - I hadn't appreciated structs can have methods. The cairo_matrix_t is a good example, thanks. Before you pointed it out I had assumed a distinguishing feature of a compact class was it had functions that took the struct as instance data

 - VAPI design choice. This is subjective of course, but I had it in my mind that those coming to Vala from languages that are not strongly influenced by C look on Vala as a more object oriented language so compact classes are often the best fit in a binding when there is a choice

> No, that's not correct. 'ref' and 'out' parameters always use the same C
> type. 
Is that not graphene_matrix_t ?

> They only differ in initialization requirements. 'ref' parameters are
> in/out parameters, which means that the caller has to initialize the
> argument before the call and the callee can but doesn't have to change the
> value. 'out' parameters must be initialized by the callee, i.e., 'out'
> parameters are semantically like extra return values.
Yes, so direction="out" caller-allocates="0" in the GIR is the same as 'out' in Vala because the callee initializes them. However, as you point out, direction="out" caller-allocates="1" is not exactly the same as an in/out parameter because the caller only allocates, not initalizes. However the C API guarantees it will initialize the argument. Is that not a pragmatic fit?

Created attachment 325864 [details]
vapigen metadata file to make Matrix a struct

astronouth7303,

Here is a metadata file to change Matrix from a compact class to a struct. Place the file in a local directory like vapi/metadata and use something like:

vapigen \
  --directory vapi \
  --metadatadir vapi/metadata/ \
  --library graphene-1.0 \
  graphene/src/Graphene-1.0.gir

What is interesting is the signature for multiply() has become:
public Graphene.Matrix multiply (Graphene.Matrix b);

Looking at https://github.com/ebassi/graphene/blob/master/src/graphene-alloc.c it appears, depending on platform, a number of memory allocation functions could be used:
 - posix_memalign
 - aligned_alloc
 - memalign
 - malloc
All of which suggest heap allocation. So it will be interesting to see how that is handled, and I'm not sure what the boxing from GType does here.

Also the fields of Matrix remain opaque, whereas cairo_matrix_t has its fields public. So it will be interesting to see what happens there.

I've also edited the pages on the wiki about generating VAPIs from GIRs to hopefully give a little better overview of the process.

Matrix works fine as a struct. That cleans up all the issues discussed here (although there's some syntactic roughness with regards to inline construction, requiring the use of temporary buffer variables).

In IRC, it was stated that it's OK for certain input, vapigen will produce unusable bindings.

Note that for the specific case of Graphene, Rico Tzschichholz has a branch in git with bindings. I'm using that in the future. https://git.gnome.org/browse/vala/log/?h=wip/gsk There is some discussion on whether compact classes are most appropriate are most appropriate for this case. They both technically work, though, with the appropriate metadata. The metadata for structs is simpler, though.