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
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 inter
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
>>
>> 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);
>>> +
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
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 *interfa
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
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);
> +
