On 07/22/2015 07:43 PM, Derek Foreman wrote:
On 22/07/15 02:25 AM, Marek Chalupa wrote:
Thanks for review,

On 07/17/2015 11:02 PM, Derek Foreman wrote:
On 16/07/15 06:59 AM, Marek Chalupa wrote:
Create functions for structures allocation (instead of inlining it into
the code) and free the objects after we don't use them anymore.

Signed-off-by: Marek Chalupa <mchqwe...@gmail.com>

I think this is quite nice, but I'd like to see the leak fixes in a
second patch, and no functional changes in the refactor patch.

Also,
if (foo)
     free(foo);

might as well be
free(foo);

since free(NULL); is totally fine.  I think the result looks cleaner.


I'm not sure what places you think. There's only

if (description)
   free_description(description)

because free_description dereferences the description, thus we need to
check if it is NULL. I could move the check into the function though.

Yup, you're obviously right.  Sorry for the line noise.

I guess maybe it's nicer if the free_foo() functions have the same
semantics as free(), but I leave that up to you.  I'm ok with it either way.


Since in this case it is valid for description to be NULL, free_description() should count with that, so yes, it is probably better.

(No further comments below)

---
   src/scanner.c | 266
+++++++++++++++++++++++++++++++++++++++++++++-------------
   1 file changed, 209 insertions(+), 57 deletions(-)

diff --git a/src/scanner.c b/src/scanner.c
index 7d8cfb9..c652612 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
@@ -316,6 +317,185 @@ is_nullable_type(struct arg *arg)
   }

   static void
+free_description(struct description *desc)
+{
+    free(desc->summary);
+    free(desc->text);
+
+    free(desc);
+}
+
+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;
+}
+
+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;
+
+    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
+        return NULL;
+
+    return arg;
+}
+
+static void
+free_arg(struct arg *arg)
+{
+    free(arg->name);
+    free(arg->interface_name);
+    free(arg->summary);
+    free(arg);
+}
+
+static void
+free_message(struct message *message)
+{
+    struct arg *a, *a_next;
+
+    free(message->name);
+    free(message->uppercase_name);
+    if (message->description)
+        free_description(message->description);
+
+    wl_list_for_each_safe(a, a_next, &message->arg_list, link)
+        free_arg(a);
+
+    free(message);
+}
+
+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);
+
+    return entry;
+}
+
+static void
+free_entry(struct entry *entry)
+{
+    free(entry->name);
+    free(entry->uppercase_name);
+    free(entry->value);
+    free(entry->summary);
+
+    free(entry);
+}
+
+static void
+free_enumeration(struct enumeration *enumeration)
+{
+    struct entry *e, *e_next;
+
+    free(enumeration->name);
+    free(enumeration->uppercase_name);
+
+    if (enumeration->description)
+        free_description(enumeration->description);
+
+    wl_list_for_each_safe(e, e_next, &enumeration->entry_list, link)
+        free_entry(e);
+
+    free(enumeration);
+}
+
+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
+free_interface(struct interface *interface)
+{
+    struct message *m, *next_m;
+    struct enumeration *e, *next_e;
+
+    free(interface->name);
+    free(interface->uppercase_name);
+    if (interface->description)
+        free_description(interface->description);
+
+    wl_list_for_each_safe(m, next_m, &interface->request_list, link)
+        free_message(m);
+    wl_list_for_each_safe(m, next_m, &interface->event_list, link)
+        free_message(m);
+    wl_list_for_each_safe(e, next_e, &interface->enumeration_list,
link)
+        free_enumeration(e);
+
+    free(interface);
+}
+
+static void
   start_element(void *data, const char *element_name, const char **atts)
   {
       struct parse_context *ctx = data;
@@ -376,32 +556,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 +604,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 +617,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 +634,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 +643,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 +652,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
@@ -1049,7 +1184,7 @@ get_include_name(bool core, enum side side)
   static void
   emit_header(struct protocol *protocol, enum side side)
   {
-    struct interface *i;
+    struct interface *i, *i_next;
       struct wl_array types;
       const char *s = (side == SERVER) ? "SERVER" : "CLIENT";
       char **p, *prev;
@@ -1102,7 +1237,7 @@ emit_header(struct protocol *protocol, enum
side side)

       printf("\n");

-    wl_list_for_each(i, &protocol->interface_list, link) {
+    wl_list_for_each_safe(i, i_next, &protocol->interface_list, link) {

           emit_enumerations(i);

@@ -1116,6 +1251,8 @@ emit_header(struct protocol *protocol, enum
side side)
               emit_opcodes(&i->request_list, i);
               emit_stubs(&i->request_list, i);
           }
+
+        free_interface(i);
       }

       printf("#ifdef  __cplusplus\n"
@@ -1231,7 +1368,7 @@ emit_messages(struct wl_list *message_list,
   static void
   emit_code(struct protocol *protocol)
   {
-    struct interface *i;
+    struct interface *i, *next;
       struct wl_array types;
       char **p, *prev;

@@ -1266,7 +1403,7 @@ emit_code(struct protocol *protocol)
       }
       printf("};\n\n");

-    wl_list_for_each(i, &protocol->interface_list, link) {
+    wl_list_for_each_safe(i, next, &protocol->interface_list, link) {

           emit_messages(&i->request_list, i, "requests");
           emit_messages(&i->event_list, i, "events");
@@ -1289,9 +1426,22 @@ emit_code(struct protocol *protocol)
               printf("\t0, NULL,\n");

           printf("};\n\n");
+
+        /* we won't need it any further */
+        free_interface(i);
       }
   }

+static void
+free_protocol(struct protocol *protocol)
+{
+    free(protocol->name);
+    free(protocol->uppercase_name);
+    free(protocol->copyright);
+    if (protocol->description)
+        free_description(protocol->description);
+}
+
   int main(int argc, char *argv[])
   {
       struct parse_context ctx;
@@ -1415,5 +1565,7 @@ int main(int argc, char *argv[])
               break;
       }

+    free_protocol(&protocol);
+
       return 0;
   }



Thanks,
Marek


Thanks,
Marek
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to