|
Rockbox mail archiveSubject: bookmark patch review remarksbookmark patch review remarks
From: Daniel Stenberg <daniel_at_haxx.se>
Date: Tue, 9 Dec 2003 13:29:57 +0100 (CET) Hi (Benjamin)! I have some remarks about the bookmarking patch. I really like the functionality and I think it is about time we got it inserted in Rockbox. What kind of collisions does it currently have with other patches? 1 - it would be nicer if the symbols (functions and variables) didn't have such terribly long names. I suggest you use 'bm_' instead of 'bookmark_' as prefix for example, and just use shorter names in other cases. 2 - another style issue - why is the function comment headers not placed before the function? The current way that puts the first line of the function before the huge comment makes the code harder to read imho. 3 - The Rockbox default-threads have a stack size of 0x400 = 1024 bytes. bookmark_load_menu() alone has TWO arrays on the stack, each using MAX_PATH (260*2 = 520) bytes. Within that function, you call bookmark_parse() it too uses two local MAX_PATH-sized arrays on the stack... wow, now we're on 1040 bytes and I haven't mentioned all the other variables... The similar problem exists in several functions. I think this code needs to consider how to be much more conservative with the stack. I don't think all these functions need to have their own local version of two MAX_PATH arrays. Given a little thinking, I bet one or two global ones could be re-used or similar. 4 - The TREE_ATTR_BMARK should probably be 0x0C00 now. Jörg just modified how these flags work. Thanks for all your patient work on this. I'm sorry I haven't provided this feedback before. -- Daniel Stenberg -- http://rockbox.haxx.se/ -- http://daniel.haxx.se/Received on 2003-12-09 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |