From: Nicolas Pennequin <nicolas.pennequin_at_free.fr>
Date: Fri, 30 Mar 2007 15:04:06 +0200

Magnus, Thanks for your review and your suggestions :)

> * Make rtc_tags const too.
Whay do you mean ? the array ? all_tags isn't const... But maybe I should make
them both const ?

> * Use for loops where appropriate (like searching the rtc_tags or
> all_tags arrays).
Not sure that'll be better. The whiles always end anyway and a for that
doesn't do all its iterations might be considered "unclean".

> * To avoid warnings about unused arguments, a common idiom is "(void)
> arg;".
Done, thanks.

> * How is show_sb_on_wps set to false? By clearing the wps_data struct
> somewhere?
Yes, the whole wps_data is cleared when the WPS is reset. Maybe that's not a
very clear way of doing things ?

> * Factor out the "get filename" code for images.
Done, thanks.

> * Is there any sane way to avoid the gotos in wps_parse? E.g., by
> calling a function or something?
I've removed one of them by factoring out another blob of dublicate code
(the "skip end of line" loop). To me, the other one seems the cleanest way of
doing what's needed, and it was said quite recently that goto's weren't
disallowed, and even encouraged when a good solution. I feel this goto is the
right solution here.

> * Consider manual common subexpression elimination in some places, like
> in the conditionals in draw_progress_bar. In my experience, GCC doesn't
> always do it where you might expect it to, and it can increase
> readability (shorter expressions).
What's "manual common subexpression elimination" ?

> * Shouldn't "while(wps_string " be "while(*wps_string "?
There were both : "while(wps_string && *wps_string ".
I moved the first one out of the while construct.

About, the archos player, what should I do ? Can I commit without really
checking the progressbars but making sure the default WPS works fine (in the
sim) ? There aren't any custom WPSs in the wiki anyway and it seems there
aren't many testers out there.

Nicolas
Received on 2007-03-30