On Mon, Jul 27, 2015 at 09:17:15AM +0200, Marek Chalupa wrote: > 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?
A followup patch to add the xzalloc cleanup would be fine. I think that'd be more robust and simplify the create routines a good bit. Bryce _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel