|
Rockbox mail archiveSubject: Re: re r28480Re: 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: > 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 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |