From: Jeff Goode <jeffg7_at_gmail.com>
Date: Tue, 06 Apr 2010 16:30:55 -0400

On 4/6/2010 12:33, Magnus Holmgren wrote:
> On 2010-04-05 22:53, mailer_at_svn.rockbox.org wrote:
>
>> New Revision: 25494
>>
>> Log Message: Restructure some bookmarking code, preparatory to adding
>> version info to bookmarks. Saves some bin size as a bonus. No
>> functional changes yet.
>
> Some comments, though I don't know exactly what you plan to change
> (and are you aware of FS#9407?):
>
> Hm, are the flags really needed? With the struct, it seems like
> F_BMFILES flag is the only one... But if they are kept, longer names
> would be nice.
>
> Why not pass struct bookmark_values as an argument? No need to keep it
> as a static then, and safe to skip (most of) the flags.
>
> resume_first_index can be removed, it isn't used.
>
> Finally, I prefer to not "hide" function calls in macros. Doesn't seem
> motivated here, IMHO.
>
I posted a patch at FS#11178 to show what I had in mind. That probably
shows where I want to go with it better than text.

The flags are required to tell parse_bookmarks which tokens to assign to
the variables. On second thought, there's no good reason not to
populate them all the time. It doesn't really hurt anything to do so.
Some flags will be necessary to process optional tokens. The variables
are in a struct only because the original plan was to pass the struct as
a parameter, but a list of static variables would do just as well.

first_index is gone in the version I working on now.

I macro'd the check_bookmark function to make it clearer what was trying
to be accomplished and to get rid of the actual function.
Received on 2010-04-06