Re: Issue 1320: Scheme bar line interface (issue 6305115)

[Marc Hohl]

Am 20.06.2012 21:13, schrieb address@hidden:
> Good work - two overall things.
>
> 1) In Scheme, try to avoid `do'. Use map and reduce.
Well, I am not very familiar with scheme, and moreover, the whole
stencil combination stuff will be replaced anyway in the second step – see
http://lists.gnu.org/archive/html/lilypond-devel/2012-06/msg00066.html

> 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.
No, not yet.
> 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.
As far as I see, Bar_line::non_empty_barline is used
in lily/note-spacing.cc and lily/paper-column.cc.
I don't know how to redefine this in Scheme and make it
available in these .cc files.
> 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.
Same as above.
> 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)
The definition of interval-widen requires the first argument to
be an interval, which half-staff isn't :-(
> 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).
Thanks, done.
> 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.
Uh? I'll give it a try later, when more urgent problems are solved.
> 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?
As said above: the whole stencil combination stuff is meant to
be replaced completely. The main idea here is to translate c++ into
scheme mostly 1:1 for better comparison.
> 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?
Done.
> http://codereview.appspot.com/6305115/