[Top][All Lists]
[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/