On Tue, 28 Apr 2015 22:57:17 +0300
Giulio Camuffo <giuliocamu...@gmail.com> wrote:
> When using this new option the generated code will include the new
> core headers instead of the old ones. The default needs to remain
> unchanged for backward compatibility with old code.
> The option handling logic code in comes directly from weston's
> shared/option-parser.c.
Hi,
Weston had a good reason to not use getopt() I believe: each module
parses its own arguments in turn, so command line options are parsed in
several passes, and an unknown option is an error only when all modules
have had a go.
getopt_long() is a GNU extension, but libwayland is already littered
with _GNU_SOURCE.
Would it make sense to use getopt_long() instead of copying Weston's
pecualiarities?
> ---
>
> v2: - rename --include-core-headers to --include-core-only
> - show the new option in the help
> - use a function to get the headers name instead of the nested ternaries
>
> src/scanner.c | 259
> ++++++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 207 insertions(+), 52 deletions(-)
>
> diff --git a/src/scanner.c b/src/scanner.c
> index adc9aa3..a1a1361 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -39,12 +39,16 @@ enum side {
> static int
> usage(int ret)
> {
> - fprintf(stderr, "usage: ./scanner [client-header|server-header|code]"
> + fprintf(stderr, "usage: ./scanner [OPTION]
> [client-header|server-header|code]"
> " [input_file output_file]\n");
> fprintf(stderr, "\n");
> fprintf(stderr, "Converts XML protocol descriptions supplied on "
> "stdin or input file to client\n"
> - "headers, server headers, or protocol marshalling
> code.\n");
> + "headers, server headers, or protocol marshalling
> code.\n\n");
> + fprintf(stderr, "options:\n");
> + fprintf(stderr, " --include-core-only include the core version
> of the headers,\n"
> + " that is e.g.
> wayland-client-core.h instead\n"
> + " of wayland-client.h.\n");
> exit(ret);
> }
>
> @@ -68,6 +72,7 @@ struct protocol {
> int null_run_length;
> char *copyright;
> struct description *description;
> + int include_core_headers;
bool core_headers;?
> };
>
> struct interface {
> @@ -985,10 +990,64 @@ format_copyright(const char *copyright)
> }
>
> static void
> +emit_types_forward_declarations(struct protocol *protocol,
> + struct wl_list *message_list,
> + struct wl_array *types)
> +{
> + struct message *m;
> + struct arg *a;
> + int length;
> + char **p;
> +
> + wl_list_for_each(m, message_list, link) {
> + length = 0;
> + m->all_null = 1;
> + wl_list_for_each(a, &m->arg_list, link) {
> + length++;
> + switch (a->type) {
> + case NEW_ID:
> + case OBJECT:
> + if (!a->interface_name)
> + continue;
> +
> + m->all_null = 0;
> + p = fail_on_null(wl_array_add(types, sizeof
> *p));
> + *p = a->interface_name;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (m->all_null && length > protocol->null_run_length)
> + protocol->null_run_length = length;
> + }
> +}
> +
> +static int
> +cmp_names(const void *p1, const void *p2)
> +{
> + const char * const *s1 = p1, * const *s2 = p2;
> +
> + return strcmp(*s1, *s2);
> +}
> +
> +static const char *
> +get_include_name(int core, enum side side)
> +{
> + if (side == SERVER)
> + return core ? "wayland-server-core.h" : "wayland-server.h";
> + else
> + return core ? "wayland-client-core.h" : "wayland-client.h";
> +}
> +
> +static void
> emit_header(struct protocol *protocol, enum side side)
> {
> struct interface *i;
> + struct wl_array types;
> const char *s = (side == SERVER) ? "SERVER" : "CLIENT";
> + char **p, *prev;
>
> if (protocol->copyright)
> format_copyright(protocol->copyright);
> @@ -1007,17 +1066,39 @@ emit_header(struct protocol *protocol, enum side side)
> "struct wl_resource;\n\n",
> protocol->uppercase_name, s,
> protocol->uppercase_name, s,
> - (side == SERVER) ? "wayland-server.h" : "wayland-client.h");
> + get_include_name(protocol->include_core_headers, side));
>
> - wl_list_for_each(i, &protocol->interface_list, link)
> - printf("struct %s;\n", i->name);
> - printf("\n");
> + wl_array_init(&types);
> + wl_list_for_each(i, &protocol->interface_list, link) {
> + emit_types_forward_declarations(protocol, &i->request_list,
> &types);
> + emit_types_forward_declarations(protocol, &i->event_list,
> &types);
> + }
This collects all the object types from arguments...
>
> wl_list_for_each(i, &protocol->interface_list, link) {
> + p = fail_on_null(wl_array_add(&types, sizeof *p));
> + *p = i->name;
> + }
...and this collects all object types from new interfaces themselves.
Ok.
> +
> + qsort(types.data, types.size / sizeof *p, sizeof *p, cmp_names);
> + prev = NULL;
> + wl_array_for_each(p, &types) {
> + if (prev && strcmp(*p, prev) == 0)
> + continue;
> + printf("struct %s;\n", *p);
> + prev = *p;
> + }
> + printf("\n");
And then you emit a sorted list of them with duplicates removed. A
useful change.
You do not mention this in the commit message, though.
After a bit of pondering, I understood why this is necessary: this is
how you get the forward declarations for all message arguments. Would
have been worth to note that. It is a change visible outside.
> +
> + prev = NULL;
> + wl_array_for_each(p, &types) {
> + if (prev && strcmp(*p, prev) == 0)
> + continue;
> printf("extern const struct wl_interface "
> - "%s_interface;\n",
> - i->name);
> + "%s_interface;\n", *p);
> + prev = *p;
> }
> +
> + wl_array_release(&types);
> printf("\n");
>
> wl_list_for_each(i, &protocol->interface_list, link) {
> @@ -1044,41 +1125,6 @@ emit_header(struct protocol *protocol, enum side side)
> }
>
> static void
> -emit_types_forward_declarations(struct protocol *protocol,
> - struct wl_list *message_list,
> - struct wl_array *types)
> -{
> - struct message *m;
> - struct arg *a;
> - int length;
> - char **p;
> -
> - wl_list_for_each(m, message_list, link) {
> - length = 0;
> - m->all_null = 1;
> - wl_list_for_each(a, &m->arg_list, link) {
> - length++;
> - switch (a->type) {
> - case NEW_ID:
> - case OBJECT:
> - if (!a->interface_name)
> - continue;
> -
> - m->all_null = 0;
> - p = fail_on_null(wl_array_add(types, sizeof
> *p));
> - *p = a->interface_name;
> - break;
> - default:
> - break;
> - }
> - }
> -
> - if (m->all_null && length > protocol->null_run_length)
> - protocol->null_run_length = length;
> - }
> -}
> -
> -static void
> emit_null_run(struct protocol *protocol)
> {
> int i;
> @@ -1181,14 +1227,6 @@ emit_messages(struct wl_list *message_list,
> printf("};\n\n");
> }
>
> -static int
> -cmp_names(const void *p1, const void *p2)
> -{
> - const char * const *s1 = p1, * const *s2 = p2;
> -
> - return strcmp(*s1, *s2);
> -}
> -
> static void
> emit_code(struct protocol *protocol)
> {
> @@ -1253,6 +1291,115 @@ emit_code(struct protocol *protocol)
> }
> }
>
> +enum option_type {
> + OPTION_INTEGER,
> + OPTION_UNSIGNED_INTEGER,
> + OPTION_STRING,
> + OPTION_BOOLEAN
> +};
> +
> +struct option {
> + enum option_type type;
> + const char *name;
> + int short_name;
> + void *data;
> +};
> +
> +static int
> +handle_option(const struct option *option, char *value)
> +{
> + char* p;
> +
> + switch (option->type) {
> + case OPTION_INTEGER:
> + * (int32_t *) option->data = strtol(value, &p, 0);
> + return *value && !*p;
> + case OPTION_UNSIGNED_INTEGER:
> + * (uint32_t *) option->data = strtoul(value, &p, 0);
> + return *value && !*p;
> + case OPTION_STRING:
> + * (char **) option->data = strdup(value);
> + return 1;
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +long_option(const struct option *options, int count, char *arg)
> +{
> + int k, len;
> +
> + for (k = 0; k < count; k++) {
> + if (!options[k].name)
> + continue;
> +
> + len = strlen(options[k].name);
> + if (strncmp(options[k].name, arg + 2, len) != 0)
> + continue;
> +
> + if (options[k].type == OPTION_BOOLEAN) {
> + if (!arg[len + 2]) {
> + * (int32_t *) options[k].data = 1;
> +
> + return 1;
> + }
> + } else if (arg[len+2] == '=') {
> + return handle_option(options + k, arg + len + 3);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +short_option(const struct option *options, int count, char *arg)
> +{
> + int k;
> +
> + if (!arg[1])
> + return 0;
> +
> + for (k = 0; k < count; k++) {
> + if (options[k].short_name != arg[1])
> + continue;
> +
> + if (options[k].type == OPTION_BOOLEAN) {
> + if (!arg[2]) {
> + * (int32_t *) options[k].data = 1;
> +
> + return 1;
> + }
> + } else {
> + return handle_option(options + k, arg + 2);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +parse_options(const struct option *options,
> + int count, int *argc, char *argv[])
> +{
> + int i, j;
> +
> + for (i = 1, j = 1; i < *argc; i++) {
> + if (argv[i][0] == '-') {
> + if (argv[i][1] == '-') {
> + if (long_option(options, count, argv[i]))
> + continue;
> + } else if (short_option(options, count, argv[i]))
> + continue;
> + }
> + argv[j++] = argv[i];
> + }
> + argv[j] = NULL;
> + *argc = j;
> +
> + return j;
> +}
Since all we need is another boolean option, using getopt_long should
make all this much less code.
> +
> int main(int argc, char *argv[])
> {
> struct parse_context ctx;
> @@ -1260,15 +1407,22 @@ int main(int argc, char *argv[])
> FILE *input = stdin;
> int len;
> void *buf;
> + int help = 0, include_core = 0;
> enum {
> CLIENT_HEADER,
> SERVER_HEADER,
> CODE,
> } mode;
>
> + const struct option options[] = {
> + { OPTION_BOOLEAN, "help", 'h', &help },
> + { OPTION_BOOLEAN, "include-core-only", 0, &include_core },
> + };
> + parse_options(options, sizeof(options) / sizeof(options[0]), &argc,
> argv);
> +
> if (argc != 2 && argc != 4)
> usage(EXIT_FAILURE);
> - else if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help") == 0)
> + else if (strcmp(argv[1], "help") == 0 || help)
> usage(EXIT_SUCCESS);
> else if (strcmp(argv[1], "client-header") == 0)
> mode = CLIENT_HEADER;
> @@ -1297,6 +1451,7 @@ int main(int argc, char *argv[])
> protocol.type_index = 0;
> protocol.null_run_length = 0;
> protocol.copyright = NULL;
> + protocol.include_core_headers = include_core;
> memset(&ctx, 0, sizeof ctx);
> ctx.protocol = &protocol;
>
I have checked that the generated headers and code from wayland.xml
do not change without the new option, excluding the the order of some
declarations which is insignificant.
Also I checked that the difference between without and with
--include-core-only indeed is just the header to be included.
Techincal comments aside, this patch is:
Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
Thanks,
pq
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
