lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Fix 1382 (issue3100041)


From: v . villenave
Subject: Re: Fix 1382 (issue3100041)
Date: Sun, 14 Nov 2010 11:22:44 +0000

Hi Carl,

looks good to me! I just have minor questions wrt coding style, but
these are mainly because of my own ignorance :)


http://codereview.appspot.com/3100041/diff/2001/input/regression/zero_staff_space.ly
File input/regression/zero_staff_space.ly (right):

http://codereview.appspot.com/3100041/diff/2001/input/regression/zero_staff_space.ly#newcode12
input/regression/zero_staff_space.ly:12: \override StaffSymbol
#'staff-space = #0.0
Just a question: why did you chose to use "0.0" instead of plain "0"
here?

http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer.cc
File lily/staff-symbol-referencer.cc (right):

http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer.cc#newcode87
lily/staff-symbol-referencer.cc:87: Real denom =
Staff_symbol::staff_space (st);
Is there a reason why you're using "denom" instead of "denominator"?
From what I can see Lily's source code doesn't use this abbrev. anywhere
else...

http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer.cc#newcode90
lily/staff-symbol-referencer.cc:90: st->warning (_ ("staff-space is 0,
setting staff-position to 0"));
I may be completely deluded here, but wouldn't it be cleaner to put
property names as %d variables in the warning string? This could make it
easier (for localization, etc.) if we were to change the property names
in the future.

http://codereview.appspot.com/3100041/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]