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
