[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Issue 1320: Scheme bar line interface (issue 6305115)
From: |
mtsolo |
Subject: |
Issue 1320: Scheme bar line interface (issue 6305115) |
Date: |
Wed, 20 Jun 2012 19:13:01 +0000 |
Good work - two overall things.
1) In Scheme, try to avoid `do'. Use map and reduce.
2) Have you tested performance hits on large scores? It may be worth it
to leave bar-line.cc as the default and have your script as a Scheme
implementation, not unlike the way we do bezier curves.
Cheers,
MS
http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc
File lily/bar-line.cc (right):
http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc#newcode31
lily/bar-line.cc:31: #include <set>
Is it worth it to keep this file? I'd just move what's left over to
Scheme at that point.
http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc
File lily/span-bar.cc (right):
http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc#newcode36
lily/span-bar.cc:36: Pointer_group_interface::add_grob (me,
ly_symbol2scm ("elements"), b);
Ditto - you can likely get rid of this as well.
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
File scm/bar-line.scm (right):
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode80
scm/bar-line.scm:80: (cons (- height half-staff) (+ height half-staff))
(interval-widen half-staff height)
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode94
scm/bar-line.scm:94: (stencil empty-stencil))
As you use let*, you can move all the set! to redefinitions of stencil
in the let*. It seems slightly more in keeping w/ coding practice (could
be wrong, though).
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode118
scm/bar-line.scm:118:
To be more scheme-ic, try a map to get an array of stencils w/ the
correct offsets and then using reduce on the list of stencils to smoosh
them together via ly:stencil-add.
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode150
scm/bar-line.scm:150: stencil)
Ditto - as a general rule, avoid `do'. Map and reduce.
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode214
scm/bar-line.scm:214: X RIGHT colon-stil kern))
Can you turn the above into the result of a call to a recursive function
that uses ly:stencil-combine-at-edge?
http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode495
scm/bar-line.scm:495: ;; bulky side. Rewritten by Han-Wen. Ported from
c++ to Scheme by Marc Hohl.
Can you get this down to the 80 character line width?
http://codereview.appspot.com/6305115/
- Issue 1320: Scheme bar line interface (issue 6305115),
mtsolo <=