Rockbox mail archive

• Next message: for_spam_at_gmx.de: "Re: Oooh look ;-)"
• Previous message: Daniel Stenberg: "Re: Oooh look ;-)"
• Next in thread: David McIntyre: "Re: bookmark patch review remarks"
• Reply: David McIntyre: "Re: bookmark patch review remarks"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] [ attachment ]

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/

• Next message: for_spam_at_gmx.de: "Re: Oooh look ;-)"
• Previous message: Daniel Stenberg: "Re: Oooh look ;-)"
• Next in thread: David McIntyre: "Re: bookmark patch review remarks"
• Reply: David McIntyre: "Re: bookmark patch review remarks"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] [ attachment ]