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
