On 5 November 2010 08:00, Thomas Martitz <thomas.mart...@student.htw-berlin.de> wrote: > 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.
it's correct. unless I'm completely missing what you are saying, last->next is set to the new object link? I actually see that is a bit redundant because we could just add to the strat of the list instead of the end, but really no big deal either way. > 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. > indeed. > > 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. > Tell you what, go and implement it better in such a way that it doesnt make dynamic screen sizing difficult or reintroduce artificial limits on the buffer size for APPLICATION builds and then we can talk, untill then, well... Read the comments in the file, if you *Actually* have an issue with it other than being snippy that I didnt ask your permission then we can talk. and by the way, it doesnt leak memory (unless I indeed did mess up the list, but I'm looking at it now and it does look correct), so unless you've run it through valgrind... Jonathan
