Created attachment 162853 [details] [review]
fix ellipsis position

Comment on attachment 162853 [details] [review]
fix ellipsis position

This patch does not make it possible to start a valist. Also from a glib view point, GError appears in g_initable_new to be right before the reference argument for the valist.

On IRC we discussed about a new syntax, "fmt...". Anyway after some research I've ended up xmlrpc_extract_method_response is an isolated example. g_initable_new has first_ parameter after GError. Vapigen automatically detects that first_ and removes it.
In other words, the patch above works enough good, except for xmlrpc_extract_method_response whose "type" parameter needs pos="-0.9".

For what concerns vala created methods, va_start() could use the error parameter. What you think of?

Using the GError doesn't fit with what /most/ people are doing. Yes, Vala could handle it, but what about other consumers of the generated API (e.g., seed, gjs, pygi, etc.)?

Most of the example I can find have the GError before the last named parameter:

gpointer            g_initable_new            (GType object_type,
                                               GCancellable *cancellable,
                                               GError **error,
                                               const gchar *first_property_name,
                                               ...);
GConfChangeSet*     gconf_engine_change_set_from_current
                                                        (GConfEngine *conf,
                                                         GError **err,
                                                         const gchar *first_key,
                                                         ...);
GConfChangeSet*     gconf_client_change_set_from_current
                                                        (GConfClient *client,
                                                         GError **err,
                                                         const gchar *first_key,
                                                         ...);
gboolean            soup_xmlrpc_extract_method_response (const char *method_response,
                                                         int length,
                                                         GError **error,
                                                         GType type,
                                                         ...);
gboolean            g_markup_collect_attributes         (const gchar *element_name,
                                                         const gchar **attribute_names,
                                                         const gchar **attribute_values,
                                                         GError **error,
                                                         GMarkupCollectType first_type,
                                                         const gchar *first_attr,
                                                         ...);
GsfOutfile *        gsf_outfile_stdio_new               (char const *root,
                                                         GError **err);
GsfOutput *         gsf_output_stdio_new_full           (char const *filename,
                                                         GError **err,
                                                         char const *first_property_name,
                                                         ...);

Although there are some where the GError is the last named parameter:

GaEntryGroupService* ga_entry_group_add_service (GaEntryGroup * group, const gchar * name, const gchar * type, guint16 port, GError ** error, ...);
GaEntryGroupService* ga_entry_group_add_service_full (GaEntryGroup * group, AvahiIfIndex 	interface, AvahiProtocol protocol, AvahiPublishFlags flags, const gchar * name, const gchar * type, const gchar * domain, const gchar * host, guint16 port, GError ** error, ...);
gboolean            gdk_pixbuf_save                     (GdkPixbuf *pixbuf,
                                                         const char *filename,
                                                         const char *type,
                                                         GError **error,
                                                         ...);
gboolean            gdk_pixbuf_save_to_buffer           (GdkPixbuf *pixbuf,
                                                         gchar **buffer,
                                                         gsize *buffer_size,
                                                         const char *type,
                                                         GError **error,
                                                         ...);

That is just from doing a `grep -n '\.\.\.' *.vapi | grep throws`. I'm sure there are other examples (for both categories). Perhaps it should be configurable for bindings, but I think for methods defined in Vala the GError should be before the last named parameter.

Review of attachment 162853 [details] [review]:

This is always needed.

*** Bug 625159 has been marked as a duplicate of this bug. ***

Created attachment 166511 [details] [review]
Fix ellipsis parameter position in generated methods.

Fixes bug 620673.

Add a test.

commit 7bb36ce26cddca52394aa4f2f85b63a834ae28ef
Author: Luca Bruno <lethalman88@gmail.com>
Date:   Sun Jul 25 09:03:31 2010 +0200

    Fix ellipsis parameter position in generated methods
    
    Fixes bug 620673.

With this patch applied, the function is generated as:

void foo (const char* msg, GError** error, ...);

instead of

void foo (GError** error, const char* msg, ...);

But the va_start call is generated as

va_start (args, msg);

Which results in "warning: second parameter of ‘va_start’ not last named argument", which I'm surprised it is only a warning.

Either va_start should be called on error instead of msg, or the error parameter should be written before the last named parameter (msg, in this case). I would prefer the latter, since that seems to be the more common C API.

commit 3cda0bdebb78d57b357ca784265cea3dd810d7f7
Author: Jürg Billeter <j@bitron.ch>
Date:   Wed Aug 1 11:39:40 2012 +0200

    codegen: Fix va_start argument in methods throwing errors
    
    Fixes bug 620673.