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

[Han-Wen Nienhuys]

[+lilypond-devel,  can this be automatic for codereviews?]

[+jan who may have more substantive arguments on signed vs. unsigned]

On Thu, Jun 17, 2010 at 12:34 AM,  <address@hidden> wrote:
>
> * 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.
>
> * 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?
>
>
> * Why are all the casts there?  Is this a 64 bit compiler thing?  Lily
> compiles virutally 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.
>
> * The unsigned vsize is something we did after moving from our
> specialized template to stl vectors (which apparently return size_t for
> some methods), and in retrospect, I think it was not a wise change, as
> it was a lot of work, has bought us little and introduces all kinds of
> bogus warnings about signed comparisons, and now -with this patch-
> introduces lots more casting.  It is probably an easier fix to change
> vsize to be an int again.
>
> * Where do you get the 5-30% number? Last time I ran a profile, time was
> dominated by get_property() and GC.  I am all for making things faster,
> but measure before optimizing.
>
>
>
>
> http://codereview.appspot.com/1724041/diff/1/59
> File lily/note-head.cc (right):
>
> http://codereview.appspot.com/1724041/diff/1/59#newcode124
> lily/note-head.cc:124: Box b = fm->get_indexed_char_dimensions (k);
> you've droppped the check here - are you sure about this?
> this is not a cosmetic change so it should be separate.
>
> http://codereview.appspot.com/1724041/show
>



-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen