Let me start by saying I know *nothing* about mensural notation.
The code looks good to me.
I found only one real issue:
LilyPond coding standards for C++ say that if there is only one
statement in an if clause, we omit {} around that clause.
I also had a question (and it probably doesn't matter much). When I've
written font glyphs that are sometimes solid and sometimes hollow, I
always calculate both the inner and outer paths. And then when I create
the glyph I only use the paths that are actually needed. In your code,
you didn't create the path unless it was needed. It probably makes no
difference at all, but I'd like to hear from the font gurus if there is
a preference.
My take was we only make the fonts at install, so the code doesn't need
to be optimized for speed, so I optimized for readability, which in my
mind, meant not putting the inner path inside a conditional.
Great work!
Thanks,
Carl
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc
File lily/mensural-ligature-engraver.cc (right):
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode277
lily/mensural-ligature-engraver.cc:277: {
Remove unneeded {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode315
lily/mensural-ligature-engraver.cc:315: prev_primitive->set_property
("add-join", ly_bool2scm (true));
Remove unneeded {} from previous line and next line
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode380
lily/mensural-ligature-engraver.cc:380: {
Why put the {} in this case?
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode448
lily/mensural-ligature-engraver.cc:448: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode453
lily/mensural-ligature-engraver.cc:453: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode455
lily/mensural-ligature-engraver.cc:455: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode460
lily/mensural-ligature-engraver.cc:460: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc
File lily/mensural-ligature.cc (right):
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc#newcode101
lily/mensural-ligature.cc:101: {
remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc#newcode144
lily/mensural-ligature.cc:144: flexa_width = robust_scm2double
(me->get_property ("flexa-width"), 2.0)
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc#newcode221
lily/mensural-ligature.cc:221: {
Remove {}
http://codereview.appspot.com/3797046/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
