Rockbox mail archive
Subject: bookmark patch review remarks
From: Daniel Stenberg (daniel_at_haxx.se)
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
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
Daniel Stenberg -- http://rockbox.haxx.se/ -- http://daniel.haxx.se/
Page was last modified "Jan 10 2012" The Rockbox Crew