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