[+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, <hanw...@gmail.com> 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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
