http://codereview.appspot.com/1724041/show>
> * can you split this up by type
of change? ie. one commit doing cosmetics of comments, one changing inline
functions, one for casts etc. This makes reviewing easier: a commit touching
only comments can be approved with
much less scrutiny than one which changes logic.
- I can, if I have to, but more than 90%
are unsigned to signed casts or changes. Almost all the comments cosmetics have
just a space or two changed, If I have to commit separately, I will drop those.
There are only 3 inline
function changes, not worth a separate patch in my opinion. But again, I will
do it, if my code reviewers want it (do they?).
> * in case you are doing cosmetic changes, can you double check that your
> standards are part of the style guide - I recall it is not that strict in
many areas, and in many cases there is no real reason to choose one over the
other. * If there is no style guide standard, can you document the style first?
- ouch ... I've just changed appearances
that looked random or unnecessary (like wrappings at ~60), or one variable
parenthesized returns etc. Should I drop all cosmetics and leave it a strict
warning correction? The code won't evolve this way, if -though being open
source- everyone wants their code to be
untouched.
> * Why are all the casts there? Is
this a 64 bit compiler thing? Lily compiles virtually without warnings over
here (core duo, gcc 4.4.4). I think all the casting hinders readability, so I
propose to not add casts unless
necessary. If the warnings bother you, add a targeted -Wno-xxx option to the
Makefile.I doubt that there are any cases where there is the risk of a real
error.
- this junks most of my patch. Many casts
were there in the first place, just in the wrong place. Ignoring warnings is
not very wise, in my opinion.The warnings should appear to you if you do a
clean and rebuild, but yes, I am using 64 bit everything.
I totally agree that this is mostly just wrinkle lift, a proper correction
would be to use signed variables (eg. first_page_num (and co.) is used as signed
and unsigned randomly). Yes, it hinders readability.
My first glance of the code suggests me (blindly) that the long long and double
are overused (I am probably wrong here), that also being the reason for all
those casts (and I also think that too many preprocessor macros are used ...
but I'm no expert, please don't take my opinion as an insult). I also
discourage using int, vsize and others as boolean (though this is also used
randomly), I think that "if (i)" should only appear if "i" is of type boolean
(but again, shouldn't one letter variables only be used as loop variables?
"Item *h", "Axis a", "Grob *g", "Real d", "Direction d" and "Direction which"
... how will I know what a "which" and a "d" is, when I see one?).
> probably an easier fix to change vsize to be an int again.
- well, that way the vector's length()
(among others) has to be casted to int, or the vector should be changed, but
the whole STL depends on unsigned variables, therefore with a custom vector
class (which wouldn't be hard or a
bad idea) we couldn't use much of the STL (I'm guessing, haven't checked how
much it is used in the code). Changing to int would also beautify the very
clever but too cryptic backwards iteration (eg. "for (vsize i = heads.size ();
i--;)").
Considering the new TR1 changes of C++0x would also be wise, though maybe
premature. A programmer shouldn't get used to anything :) (only to change :p).
> * Where do you get the 5-30% number? Last time I ran a profile, time was
- sorry if I was misunderstandable, I
only meant that those 2 functions were ~4 and ~26 percent faster in my little
test program I wrote (should I send it? I don't think it matters that much),
than their branching predecessors.
> you've dropped the check here - are you sure about this? this is not a
> cosmetic change so it should be separate.
- it *is* cosmetics, it's just that the
unmarked casting to int made it seemingly possible to be < 0, which it cannot
be, since fm->name_to_index
(key) returns an unsigned positive value anyway (and it doesn't even depend on
the key, which is quite strange to me).
size_t
Font_metric::name_to_index (string) const
{
return (size_t)-1;
}
Lőrinc
ps. added -devel to CC
_______________________________________________
lilypond-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/lilypond-devel
