Re: [PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
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.
(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
Re: [PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
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;
+
Re: [PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
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
Re: [PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
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.
(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);
+
