[PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
Derek Foreman
derekf at osg.samsung.com
Fri Jul 17 14:02:47 PDT 2015
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 <mchqwerty at 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);
> + 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;
> }
>
More information about the wayland-devel
mailing list