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 <mchqwe...@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@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
