Rockbox mail archiveSubject: Re: re r28480
Re: re r28480
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:
> 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?
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
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.
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.
Received on 2010-11-04