GNOME Bugzilla – Bug 781755
Avoid compiler warnings in generated marshallers code
Last modified: 2017-04-28 17:00:45 UTC
Right now, projects like GTK+ need to do contorsions like: (echo "#include \"gtkmarshals.h\"; \ glib-genmarshal --body …) > gtkmarshals.c when generating marshallers. This is going to be hard to replicate on any build system that is not autotools. Adding an `--include-header` command line argument to glib-genmarshal would allow us to fix this pretty easily.
Created attachment 350447 [details] [review] genmarshal: Add argument for including a header Various projects need to redirect the output of glib-genmarshal to a temporary file in order to prepend an inclusion directive for the marshallers header, in order to compile with warnings enabled.
Review of attachment 350447 [details] [review]: The new option needs to be documented in docs/reference/gobject/glib-genmarshal.xml. Would it make sense to allow this option to be specified multiple times? --include-header header1.h --include-header header2.h? ::: gobject/glib-genmarshal.c @@ +123,3 @@ static gchar * const std_marshaller_prefix = "g_cclosure_marshal"; static gchar *marshaller_prefix = "g_cclosure_user_marshal"; +static gchar *include_header = NULL; Nitpick: make it const. @@ +1025,3 @@ + (strncmp ("--include-header=", argv[i], 17) == 0)) + { + char *equal = argv[i] + 16; Nitpick: `const char *equal = argv[i] + strlen ("--include-header")` @@ +1142,3 @@ g_fprintf (bout, " --internal Mark generated functions as internal\n"); g_fprintf (bout, " --valist-marshallers Generate va_list marshallers\n"); + g_fprintf (bout, " --include-header=file The header file to include in the C code\n"); Nitpick: I think the convention would be `--include-header=FILE` (capitalisation).
I've mostly followed the extant coding style, but I can update the patch.
(In reply to Philip Withnall from comment #2) > Review of attachment 350447 [details] [review] [review]: > > The new option needs to be documented in > docs/reference/gobject/glib-genmarshal.xml. Fixed. > Would it make sense to allow this option to be specified multiple times? > --include-header header1.h --include-header header2.h? I don't really see the purpose; generally you want to include the generated marshallers header, in order to pass `-Wmissing-prototypes`, and since header and source files are generated separately, glib-genmarshal cannot predict the name of the header file by itself. Though, now that I think of it, we could literally generate the prototypes without adding an inclusion directive. > ::: gobject/glib-genmarshal.c > @@ +123,3 @@ > static gchar * const std_marshaller_prefix = "g_cclosure_marshal"; > static gchar *marshaller_prefix = "g_cclosure_user_marshal"; > +static gchar *include_header = NULL; > > Nitpick: make it const. We're not using const in other options, so I'd have to make a separate commit to avoid messing up this one. > @@ +1025,3 @@ > + (strncmp ("--include-header=", argv[i], 17) == 0)) > + { > + char *equal = argv[i] + 16; > > Nitpick: `const char *equal = argv[i] + strlen ("--include-header")` Same as above, I'd probably do a separate commit and fix other arguments. > @@ +1142,3 @@ > g_fprintf (bout, " --internal Mark generated > functions as internal\n"); > g_fprintf (bout, " --valist-marshallers Generate va_list > marshallers\n"); > + g_fprintf (bout, " --include-header=file The header file to > include in the C code\n"); > > Nitpick: I think the convention would be `--include-header=FILE` > (capitalisation). The help for `--output` uses `--output=file`. I can fix both in a separate commit.
(In reply to Emmanuele Bassi (:ebassi) from comment #4) > Though, now that I think of it, we could literally generate the prototypes > without adding an inclusion directive. That seems like a better solution, and there should be no need to support including other headers then, since the set of types glib-genmarshal supports is closed (so we don’t need to include user-provided headers to get their typedefs). Do you want to go with that approach instead? It would make using glib-genmarshal easier, rather than requiring projects to change their makefiles (or other build system goop) to pass --include-header. > > ::: gobject/glib-genmarshal.c > > @@ +123,3 @@ > > static gchar * const std_marshaller_prefix = "g_cclosure_marshal"; > > static gchar *marshaller_prefix = "g_cclosure_user_marshal"; > > +static gchar *include_header = NULL; > > > > Nitpick: make it const. > > We're not using const in other options, so I'd have to make a separate > commit to avoid messing up this one. +1 for doing this as a separate commit, regardless of whether we go with --include-headers or generating the prototypes in the C file. > > @@ +1025,3 @@ > > + (strncmp ("--include-header=", argv[i], 17) == 0)) > > + { > > + char *equal = argv[i] + 16; > > > > Nitpick: `const char *equal = argv[i] + strlen ("--include-header")` > > Same as above, I'd probably do a separate commit and fix other arguments. +1 similarly. > > @@ +1142,3 @@ > > g_fprintf (bout, " --internal Mark generated > > functions as internal\n"); > > g_fprintf (bout, " --valist-marshallers Generate va_list > > marshallers\n"); > > + g_fprintf (bout, " --include-header=file The header file to > > include in the C code\n"); > > > > Nitpick: I think the convention would be `--include-header=FILE` > > (capitalisation). > > The help for `--output` uses `--output=file`. I can fix both in a separate > commit. +1 similarly, though I am less bothered about this as long as we’re consistent.
Created attachment 350471 [details] [review] genmarshal: Add argument for including a header Various projects need to redirect the output of glib-genmarshal to a temporary file in order to prepend an inclusion directive for the marshallers header, in order to compile with warnings enabled.
Created attachment 350472 [details] [review] genmarshal: Conform --help output to conventions The convention for arguments taking a value is: --argument=VALUE with the `VALUE` in caps.
Created attachment 350473 [details] [review] genmarshal: Constify global variables
Created attachment 350474 [details] [review] genmarshal: Use fewer magic numbers
Created attachment 350475 [details] [review] genmarshal: Always generate the prototypes in the body This way code that does not pass `--include-header` and builds with `-Wmissing-prototypes` will not generate a compiler warning.
Created attachment 350476 [details] [review] genmarshal: Always generate the prototypes in the body This way code that does not manually include the generated marshallers header and wishes to build with `-Wmissing-prototypes` will not generate a compiler warning.
I've implemented both approaches. We can commit attachment 350476 [details] [review] and fix the warnings without adding a new command line argument; we can also commit the other attachments if we want to add the --include-header argument, or simply reject them if it's not worth the pain.
Review of attachment 350472 [details] [review]: ++
Review of attachment 350473 [details] [review]: ++
Review of attachment 350474 [details] [review]: ++
Review of attachment 350476 [details] [review]: Would it make sense to factor this out from the `(gen_cheader && !have_std_marshaller)` case too, so the code is shared? ::: gobject/glib-genmarshal.c @@ +397,3 @@ + ind = g_fprintf (fout, "%s_%s (", marshaller_prefix, signame); + g_fprintf (fout, "GClosure *closure,\n"); + g_fprintf (fout, "%sGValue *return_value G_GNUC_UNUSED,\n", indent (ind)); I think this is only unused if `!sig->rarg->setter`, from looking at the code below. @@ +400,3 @@ + g_fprintf (fout, "%sguint n_param_values,\n", indent (ind)); + g_fprintf (fout, "%sconst GValue *param_values,\n", indent (ind)); + g_fprintf (fout, "%sgpointer invocation_hint G_GNUC_UNUSED,\n", indent (ind)); (This one’s always unused.)
Review of attachment 350471 [details] [review]: This looks OK with the other fixups you attached. Unless there’s another reason to support --include-header, though, I am in favour of the put-the-prototypes-in-the-C-file approach.
Created attachment 350563 [details] [review] genmarshal: Always generate the prototypes in the body This way code that does not manually include the generated marshallers header and wishes to build with `-Wmissing-prototypes` will not generate a compiler warning.
(In reply to Philip Withnall from comment #16) > Review of attachment 350476 [details] [review] [review]: > > Would it make sense to factor this out from the `(gen_cheader && > !have_std_marshaller)` case too, so the code is shared? Yes. > ::: gobject/glib-genmarshal.c > @@ +397,3 @@ > + ind = g_fprintf (fout, "%s_%s (", marshaller_prefix, signame); > + g_fprintf (fout, "GClosure *closure,\n"); > + g_fprintf (fout, "%sGValue *return_value G_GNUC_UNUSED,\n", > indent (ind)); > > I think this is only unused if `!sig->rarg->setter`, from looking at the > code below. Yep; fixed it.
This looks good to me, fwiw.
Review of attachment 350563 [details] [review]: Looks good.
Attachment 350472 [details] pushed as 1ffad8f - genmarshal: Conform --help output to conventions Attachment 350473 [details] pushed as a8b5192 - genmarshal: Constify global variables Attachment 350474 [details] pushed as 616cff7 - genmarshal: Use fewer magic numbers Attachment 350563 [details] pushed as 17a3c78 - genmarshal: Always generate the prototypes in the body