|
Rockbox mail archiveSubject: Re: WPS tokenizerRe: WPS tokenizer
From: Magnus Holmgren <lear_at_algonet.se>
Date: Fri, 30 Mar 2007 18:40:00 +0200 Nicolas Pennequin wrote: >> * Make rtc_tags const too. > Whay do you mean ? the array ? all_tags isn't const... But maybe I should make > them both const ? Yes, arrays with constant data should be 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". It's common with early termination of for loops, so I don't see that as unclean. Rather, when I first looked at the rtc_tags search loop, I didn't realize what it was, because it was a while rather than a for. >> * 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 ? Not bad as such. I noticed that function only set the field to true, but I couldn't (easily) see where it was set to false. A comment stating that it defaults to false would be enough. >> * 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. Yes, gotos can be acceptable (especially for error handling), but having it jump around within a switch should be discouraged, IMHO, particularly if the goto label almost look like another "case" statement. :) Reducing the indent of the labels would help, to make it clear there's something unusual going on. >> * 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" ? An optimization technique used by compilers, where if a subexpression (e.g., "a * b" in ("a * b + c * d") is used in several places, the compiler does the calculation once, and stores the result in a temporary variable, which it then uses in place of that sub-expression. The case I was referring to in draw_progress_bar is the "state->id3->length" thing, which is used in 6 places. replacing that with a "len" field could increase readability and perhaps save a few bytes. Maybe not worth it here, but as an example, there's an if-statement in filetree.c where the expression "(dptr->attr & TREE_ATTR_MASK)" is used 12 times. Last time I checked (on m68k-elf, using gcc 3.4.x), that expression was actually executed 12 times. > 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. Since you can't test it, there's not much you can do - unless someone volunteers to test the patch for you. Magnus Received on 2007-03-30 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |