[PATCH wayland v2 1/2] scanner: refactor creating objects
Marek Chalupa
mchqwerty at gmail.com
Mon Jul 27 00:17:15 PDT 2015
Hi,
thanks for review.
On 07/23/2015 08:41 PM, Bryce Harrington wrote:
> 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.
These are initialized right after create_message().
I didn't want to initialize them to some default value and in the very
next few lines of code rewrite them again.
>
> 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.
Yes, but number of items in the structures that are initialized to 0's
is not so big. struct message has 3 fields initialized to 0 out of 12,
struct arg 2 of 6, ... (see the end of the e-mail)
>
> 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
initialized again right after create_arg
>
>> + 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...
Yes, has this in the patch no. 2, but it should be here
>
>> + 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.
Actually, this returns NULL only because I didn't want to pass to
ctx->loc to create_arg. I think that quite clear solution will be
to move setting the argument type to self-standing function, that
can return boolean.
>
>> +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
I'm not against using xzalloc and initialize all new memory to 0, it can
make the code more clean. But the intention of this patch was to 'move'
the current code into the functions, without any functional changes - if
you would inline the create_* functions, you'd get the old code (well,
except for like 3-5 lines). There is a lot of places with nested if's
that are setting the field to either 0 or 1, so initializing all to 0's
and then removing the branches that set the fields to 0 could introduce
bugs. Therefore I think it would be better to do it as a follow up,
wouldn't be?
Thanks,
Marek
More information about the wayland-devel
mailing list