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.

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

Reply via email to