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