From: Thomas Martitz <thomas.martitz_at_student.htw-berlin.de>
Date: Thu, 04 Nov 2010 22:00:59 +0100

Am 04.11.2010 16:30, schrieb Al Le:
> Hello.
>
> I have a couple of questions / suggestions about the patch.
>
> 1. Shouldn't the vars 'first' and 'last' be declared static? And renamed to something more specific, e.g. 'alloced_list_head' and '..._tail'?
>
> 2. Shouldn't the result of the call to malloc be casted to the desired pointer type?
>
> 3. I think, the list tail is not properly updated in the malloc function for the 'USE_HOST_MALLOC' branch
>
> 4. The result of one malloc call is not checked to be not NULL (the object itself).
>
> 5. The function skin_free_malloced could be made static.
>
> Are these points valid?
>

1) Sure
2) Actually no. You don't need to cast void*, and casting can hide
errors (e.g. if you cast to a typedef and the typedef isn't actually
pointer).
3) Yes, it looks broken to me as well. Good catch.
4) the object is returned, doesn't matter if it's NULL as it's the
callers responsibility to check for NULL anyway.
5) Yea

Additionally, I think tiny mallocs should be avoided if possible, the
two mallocs in that function (one of them allocs 4 bytes) could be
merged to one. But I think that's my own opinion.


This commit was made in a rush, I saw no sign of discussion between the
first proof-of-concept-paste on pastebin and the commit 2h after. It
leaks memory, you lost the ability to track the skin buffer usage and
this the theme size, and generally I think introducing malloc in core
code should deserve more discussion even if it's on hosted targets.

Best regards.
Received on 2010-11-04