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); +