Re: 100+ warnings and some cosmetics along the way (issue1724041)

[Pap Lôrinc]

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