[PATCH wayland v2 1/2] scanner: refactor creating objects
Bryce Harrington
bryce at osg.samsung.com
Thu Jul 23 11:41:47 PDT 2015
On Thu, Jul 23, 2015 at 07:39:30AM +0200, Marek Chalupa wrote:
> wrap creating and initializing objects (structures)
> into functions and use them in the code.
>
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
> src/scanner.c | 158 ++++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 105 insertions(+), 53 deletions(-)
>
> diff --git a/src/scanner.c b/src/scanner.c
> index 7d8cfb9..85c283b 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -1,6 +1,7 @@
> /*
> * Copyright © 2008-2011 Kristian Høgsberg
> * Copyright © 2011 Intel Corporation
> + * Copyright © 2015 Red Hat, Inc.
> *
> * Permission is hereby granted, free of charge, to any person obtaining
> * a copy of this software and associated documentation files (the
> @@ -315,6 +316,102 @@ is_nullable_type(struct arg *arg)
> }
> }
>
> +static struct message *
> +create_message(struct location loc, const char *name)
> +{
> + struct message *message;
> +
> + message = xmalloc(sizeof *message);
> + message->loc = loc;
> + message->name = xstrdup(name);
> + message->uppercase_name = uppercase_dup(name);
> + wl_list_init(&message->arg_list);
> + message->arg_count = 0;
> + message->new_id_count = 0;
> + message->description = NULL;
> +
> + return message;
> +}
The message structure has a few more fields, that this constructor
leaves uninitialized. I don't know if that's ok or not.
struct message {
struct location loc;
char *name;
char *uppercase_name;
struct wl_list arg_list;
--> struct wl_list link;
int arg_count;
int new_id_count;
--> int type_index;
--> int all_null;
--> int destructor;
--> int since;
struct description *description;
};
One thing you might consider is rather than xmalloc, to use calloc
which will guarantee all members are set to 0 initially. calloc can
actually be more efficient than manually setting fields to 0 since the
kernel can give us pre-zero'd memory if it has any.
Here's what we use in weston:
# from shared/zalloc.h
static inline void *
zalloc(size_t size)
{
return calloc(1, size);
}
# From window.c
void *
xzalloc(size_t s)
{
return fail_on_null(zalloc(s));
}
With these definitions you could essentially s/xmalloc/xzalloc/
throughout the scanner code, and then drop any lines that just
initialize member fields to NULL or 0, as they will be redundant.
> +
> +static struct arg *
> +create_arg(const char *name, const char *type)
> +{
> + struct arg *arg;
> +
> + arg = xmalloc(sizeof *arg);
> + arg->name = xstrdup(name);
> + arg->summary = NULL;
> + arg->interface_name = NULL;
nullable isn't initialized
> + if (strcmp(type, "int") == 0)
> + arg->type = INT;
> + else if (strcmp(type, "uint") == 0)
> + arg->type = UNSIGNED;
> + else if (strcmp(type, "fixed") == 0)
> + arg->type = FIXED;
> + else if (strcmp(type, "string") == 0)
> + arg->type = STRING;
> + else if (strcmp(type, "array") == 0)
> + arg->type = ARRAY;
> + else if (strcmp(type, "fd") == 0)
> + arg->type = FD;
> + else if (strcmp(type, "new_id") == 0)
> + arg->type = NEW_ID;
> + else if (strcmp(type, "object") == 0)
> + arg->type = OBJECT;
> + else
should free(arg) at this point...
> + return NULL;
> +
> + return arg;
> +}
It's a little inconsistent that create_arg can return NULL when all the
other create_* routines are being guaranteed to never return NULL.
Since the only action that appears to be taken when this happens is to
fail and terminate the program, this routine could just call
fail_on_null(NULL) when it hits the unknown arg type in the if structure
above. That would make behaviors consistent.
Alternatively, the arg structure could be given an error indicator, and
then the caller check that (rather than arg == NULL). But I think this
is probably overkill.
> +static struct enumeration *
> +create_enumeration(const char *name)
> +{
> + struct enumeration *enumeration;
> +
> + enumeration = xmalloc(sizeof *enumeration);
> + enumeration->name = xstrdup(name);
> + enumeration->uppercase_name = uppercase_dup(name);
> + enumeration->description = NULL;
> +
> + wl_list_init(&enumeration->entry_list);
> +
> + return enumeration;
> +}
> +
> +static struct entry *
> +create_entry(const char *name, const char *value)
> +{
> + struct entry *entry;
> +
> + entry = xmalloc(sizeof *entry);
> + entry->name = xstrdup(name);
> + entry->uppercase_name = uppercase_dup(name);
> + entry->value = xstrdup(value);
summary isn't initialized
> +
> + return entry;
> +}
> +
> +static struct interface *
> +create_interface(struct location loc, const char *name, int version)
> +{
> + struct interface *interface;
> +
> + interface = xmalloc(sizeof *interface);
> + interface->loc = loc;
> + interface->name = xstrdup(name);
> + interface->uppercase_name = uppercase_dup(name);
> + interface->version = version;
> + interface->description = NULL;
> + interface->since = 1;
> + wl_list_init(&interface->request_list);
> + wl_list_init(&interface->event_list);
> + wl_list_init(&interface->enumeration_list);
> +
> + return interface;
> +}
> +
> static void
> start_element(void *data, const char *element_name, const char **atts)
> {
> @@ -376,32 +473,16 @@ start_element(void *data, const char *element_name, const char **atts)
> if (version == 0)
> fail(&ctx->loc, "no interface version given");
>
> - interface = xmalloc(sizeof *interface);
> - interface->loc = ctx->loc;
> - interface->name = xstrdup(name);
> - interface->uppercase_name = uppercase_dup(name);
> - interface->version = version;
> - interface->description = NULL;
> - interface->since = 1;
> - wl_list_init(&interface->request_list);
> - wl_list_init(&interface->event_list);
> - wl_list_init(&interface->enumeration_list);
> + interface = create_interface(ctx->loc, name, version);
> + ctx->interface = interface;
> wl_list_insert(ctx->protocol->interface_list.prev,
> &interface->link);
> - ctx->interface = interface;
> } else if (strcmp(element_name, "request") == 0 ||
> strcmp(element_name, "event") == 0) {
> if (name == NULL)
> fail(&ctx->loc, "no request name given");
>
> - message = xmalloc(sizeof *message);
> - message->loc = ctx->loc;
> - message->name = xstrdup(name);
> - message->uppercase_name = uppercase_dup(name);
> - wl_list_init(&message->arg_list);
> - message->arg_count = 0;
> - message->new_id_count = 0;
> - message->description = NULL;
> + message = create_message(ctx->loc, name);
>
> if (strcmp(element_name, "request") == 0)
> wl_list_insert(ctx->interface->request_list.prev,
> @@ -440,28 +521,9 @@ start_element(void *data, const char *element_name, const char **atts)
> if (name == NULL)
> fail(&ctx->loc, "no argument name given");
>
> - arg = xmalloc(sizeof *arg);
> - arg->name = xstrdup(name);
> -
> - if (strcmp(type, "int") == 0)
> - arg->type = INT;
> - else if (strcmp(type, "uint") == 0)
> - arg->type = UNSIGNED;
> - else if (strcmp(type, "fixed") == 0)
> - arg->type = FIXED;
> - else if (strcmp(type, "string") == 0)
> - arg->type = STRING;
> - else if (strcmp(type, "array") == 0)
> - arg->type = ARRAY;
> - else if (strcmp(type, "fd") == 0)
> - arg->type = FD;
> - else if (strcmp(type, "new_id") == 0) {
> - arg->type = NEW_ID;
> - } else if (strcmp(type, "object") == 0) {
> - arg->type = OBJECT;
> - } else {
> + arg = create_arg(name, type);
> + if (!arg)
> fail(&ctx->loc, "unknown type (%s)", type);
> - }
>
> switch (arg->type) {
> case NEW_ID:
> @@ -472,8 +534,6 @@ start_element(void *data, const char *element_name, const char **atts)
> case OBJECT:
> if (interface_name)
> arg->interface_name = xstrdup(interface_name);
> - else
> - arg->interface_name = NULL;
> break;
> default:
> if (interface_name != NULL)
> @@ -491,7 +551,6 @@ start_element(void *data, const char *element_name, const char **atts)
> if (allow_null != NULL && !is_nullable_type(arg))
> fail(&ctx->loc, "allow-null is only valid for objects, strings, and arrays");
>
> - arg->summary = NULL;
> if (summary)
> arg->summary = xstrdup(summary);
>
> @@ -501,12 +560,7 @@ start_element(void *data, const char *element_name, const char **atts)
> if (name == NULL)
> fail(&ctx->loc, "no enum name given");
>
> - enumeration = xmalloc(sizeof *enumeration);
> - enumeration->name = xstrdup(name);
> - enumeration->uppercase_name = uppercase_dup(name);
> - enumeration->description = NULL;
> - wl_list_init(&enumeration->entry_list);
> -
> + enumeration = create_enumeration(name);
> wl_list_insert(ctx->interface->enumeration_list.prev,
> &enumeration->link);
>
> @@ -515,10 +569,8 @@ start_element(void *data, const char *element_name, const char **atts)
> if (name == NULL)
> fail(&ctx->loc, "no entry name given");
>
> - entry = xmalloc(sizeof *entry);
> - entry->name = xstrdup(name);
> - entry->uppercase_name = uppercase_dup(name);
> - entry->value = xstrdup(value);
> + entry = create_entry(name, value);
> +
> if (summary)
> entry->summary = xstrdup(summary);
> else
> --
> 2.4.3
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list