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