Creates pure closures

Creates pure closures

[MikeSol, 2011-08-18 09:21:59]

This is a more extensible way to deal with pure properties.  I'd like this patch
to be the first step, with the second step being rewriting
define-grob-properties.scm such that it uses pure closures as much as possible. 
First, create a procedure:

#(define-public (pure-wrapper proc)
  (lambda (grob start end) (proc grob)))

Then, for example:

(stencil . ,ly:clef::print)

becomes

(stencil . ,(ly:pure-closure ly:clef::print (pure-wrapper ly:clef::print)))

...

(Y-extent . ,ly:stem::height)

becomes

(Y-extent . ,(ly:pure-closure ly:stem::height ly:stem::pure-height))

etc.

It'd allow the elimination of the lists down in the bottom of define-grobs.scm
and would allow call-pure-function to be a lot smaller.

The reason I'm doing this is in anticipation of my work with stems.  Given that
the user will be overriding properties that have pure conversions, it seems
logical that the user would be able to define pure conversions for said
overrides.

Cheers,
MS

[reinhold_kainhofer.com, 2011-08-18 11:06:57]

Am Thursday, 18. August 2011, 11:21:59 schrieb mtsolo@gmail.com:
> This is a more extensible way to deal with pure properties.  I'd like
> this patch to be the first step, with the second step being rewriting
> define-grob-properties.scm such that it uses pure closures as much as
> possible.  First, create a procedure:
> 
> #(define-public (pure-wrapper proc)
>    (lambda (grob start end) (proc grob)))
> 
> Then, for example:
> 
> (stencil . ,ly:clef::print)
> 
> becomes
> 
> (stencil . ,(ly:pure-closure ly:clef::print (pure-wrapper
> ly:clef::print)))

Wow, that would be VERY user-unfriendly. Just imagine explaining that to 
someone on -user... 
(or even to the average lilypond developer... I still haven't understood what 
closures are).

Cheers,
Reinhold
-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[Neil Puttock, 2011-08-18 12:05:46]

On 2011/08/18 11:06:57, reinhold_kainhofer.com wrote:

> Wow, that would be VERY user-unfriendly. Just imagine explaining that to 
> someone on -user... 
> (or even to the average lilypond developer... I still haven't understood what 
> closures are).

I agree, though I do like the idea in principle since it would allow more
complicated stencil overrides to work properly with skylining (e.g., custom
notehead callbacks which currently break accidental positioning).

Cheers,
Neil

[Neil Puttock, 2011-08-18 12:31:27]

Hi Mike,

I have reservations about the naming, since you're basically creating a smob
which acts as a container for a pair of callbacks; it doesn't work like a
simple-closure in that you can evaluate the closure and get something useful
back.

Cheers,
Neil

http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc
File lily/pure-closure.cc (right):

http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc#newcode65
lily/pure-closure.cc:65: 2, 0, 0, (SCM pc),
1, 0, 0,

(there's only one arg; looks like you've been cut'n'pasting from
simple-closure.cc :)

http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc#newcode73
lily/pure-closure.cc:73: 2, 0, 0, (SCM pc),
1, 0, 0,

http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc#newcode84
lily/pure-closure.cc:84: scm_display (scm_cdr (s), port);
this only displays the unpure part, and if you change it to show both, there
seems to be a garbage collection problem (probably to do with using scm_markcdr
in init_pure_closure ()); the pure part has been swept away:

\relative c' {
  \override Stem #'Y-extent =
    #(ly:make-pure-closure ly:stem::height '(-3 . 10))
  c4
}

\layout {
  \context {
    \Voice
    \override Stem #'after-line-breaking =
    #(lambda (grob)
       (let ((data (ly:grob-property-data grob 'Y-extent)))
         (and (ly:pure-closure? data)
              (newline)
              (display data))))
  }
}

-> #<pure-closure #<primitive-procedure ly:stem::height> #<freed cell
0x7f4148a3f960; GC missed a reference> >

(the wrapper you posted fails even for a \once \override, and would be bad for
documentation purposes since the pure part would show as an anonymous lambda)

[mikesol_ufl.edu, 2011-08-18 12:31:59]

On Aug 18, 2011, at 1:06 PM, Reinhold Kainhofer wrote:

> Am Thursday, 18. August 2011, 11:21:59 schrieb mtsolo@gmail.com:
>> This is a more extensible way to deal with pure properties.  I'd like
>> this patch to be the first step, with the second step being rewriting
>> define-grob-properties.scm such that it uses pure closures as much as
>> possible.  First, create a procedure:
>> 
>> #(define-public (pure-wrapper proc)
>>   (lambda (grob start end) (proc grob)))
>> 
>> Then, for example:
>> 
>> (stencil . ,ly:clef::print)
>> 
>> becomes
>> 
>> (stencil . ,(ly:pure-closure ly:clef::print (pure-wrapper
>> ly:clef::print)))
> 
> Wow, that would be VERY user-unfriendly. Just imagine explaining that to 
> someone on -user... 
> (or even to the average lilypond developer... I still haven't understood what 
> closures are).
> 

I think it is rather easy (and necessary) to explain this to a user, especially
if there are helper functions.

Example:

in output-lib.scm, write

#(define-public (pure-wrapper proc)
  (lambda (grob start end) (proc grob)))

#(define-public (simple-pure proc)
  (ly:pure-closure proc (pure-wrapper proc))

#(define-public (simple-pure-height proc)
  (ly:pure-closure ly:grob::stencil-height (pure-wrapper proc))

The, in the documentations on overrides:

Sometimes, you will want to override a stencil and will want this stencil to be
taken into account when LilyPond does horizontal spacing.  For LilyPond to know
that a stencil will not lead to things going bad (i.e. triggering a calculation
that should happen at the end of a series of calculations at the beginning), it
needs what are called "pure" approximations of grobs.  If you know that your
grob's height will be pure (meaning it can be calculated without triggering lots
of other calculations in other grobs), you can help the horizontal spacing
engine by adding the following override:

\override Clef #'stencil = #my-stencil-proc
\override Clef #'Y-extent = #(simple-pure-height my-stencil-proc)

Of course, if you think that your stencil function will wreak havoc on other
grobs, don't use simple-pure-height.  Some suggestions:

1)  If your stencil does not use the grob at all and just draws shapes, you can
use simple-pure-height.
2)  If you are positive that your stencil does not trigger a VerticalAlignment
grob, you can use simple-pure-height.

A good way to know if your stencil is pure or not is to run it through LilyPond.

#(define (my-good-unpure-property grob) (ly:stem::height grob))
#(define (my-bad-pure-property grob start end) (ly:stem::height grob))
#(define (my-good-pure-property grob start end) (ly:stem::pure-height grob start
end))

First, try:

\relative c' {
  \override Stem #'Y-extent =
    #(ly:make-pure-closure my-good-unpure-property my-bad-pure-property)
  \repeat unfold 10 { d8 fis e gis }
}

Then, try:

\relative c' {
  \override Stem #'Y-extent =
    #(ly:make-pure-closure my-good-unpure-property my-good-pure-property)
  \repeat unfold 10 { d8 fis e gis }
}

The first will issue a series of warning messages, whereas the second will not.
***

You get the idea.

I think that, even with this introduced, the average user can keep on doing
exactly what she was doing before this patch - this just gives her the ability
to introduce her stencils into skyline calculations.

Cheers,
MS

[mikesol_ufl.edu, 2011-08-18 12:44:49]

On Aug 18, 2011, at 2:31 PM, n.puttock@gmail.com wrote:

> Hi Mike,
> 
> I have reservations about the naming, since you're basically creating a
> smob which acts as a container for a pair of callbacks; it doesn't work
> like a simple-closure in that you can evaluate the closure and get
> something useful back.
> 
> Cheers,
> Neil
> 

What about pure-container ?

> 
> http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc
> File lily/pure-closure.cc (right):
> 
> http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc#newcode65
> lily/pure-closure.cc:65: 2, 0, 0, (SCM pc),
> 1, 0, 0,
> 
> (there's only one arg; looks like you've been cut'n'pasting from
> simple-closure.cc :)
> 
> http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc#newcode73
> lily/pure-closure.cc:73: 2, 0, 0, (SCM pc),
> 1, 0, 0,
> 

Fixed and fixed - thanks!

> http://codereview.appspot.com/4894052/diff/1/lily/pure-closure.cc#newcode84
> lily/pure-closure.cc:84: scm_display (scm_cdr (s), port);
> this only displays the unpure part, and if you change it to show both,
> there seems to be a garbage collection problem (probably to do with
> using scm_markcdr in init_pure_closure ()); the pure part has been swept
> away:
> 

I posted a new patch that should fix this.

Many thanks, as always!

Cheers,
MS

[Neil Puttock, 2011-08-19 22:39:50]

http://codereview.appspot.com/4894052/diff/9001/input/regression/pure-closure.ly
File input/regression/pure-closure.ly (right):

http://codereview.appspot.com/4894052/diff/9001/input/regression/pure-closure...
input/regression/pure-closure.ly:18: #(ly:make-pure-closure ly:stem::height '(-3
. 10))
I'm a bit worried this is too close to an extra-spacing-height override to make
a good test example.

http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc
File lily/pure-closure.cc (right):

http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode36
lily/pure-closure.cc:36: return (SCM) SCM_CELL_WORD_1 (smob);
SCM_PACK (SCM_CELL_WORD_1 (smob));

(if you want to be strict :)

http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode43
lily/pure-closure.cc:43: return (SCM) SCM_CELL_WORD_2 (smob);
SCM_PACK (SCM_CELL_WORD_2 (smob));

http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode60
lily/pure-closure.cc:60: SCM_NEWSMOB2 (z, pure_closure_tag, unpure, pure);
SCM_UNPACK (unpure), SCM_UNPACK (pure)

http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode68
lily/pure-closure.cc:68: assert (is_pure_closure (pc));
optimized builds will segfault on invalid args, so you should use LY_ASSERT_TYPE
() here

http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode76
lily/pure-closure.cc:76: assert (is_pure_closure (pc));
LY_ASSERT_TYPE ()

http://codereview.appspot.com/4894052/diff/9001/scm/define-grobs.scm
File scm/define-grobs.scm (left):

http://codereview.appspot.com/4894052/diff/9001/scm/define-grobs.scm#oldcode2655
scm/define-grobs.scm:2655: (if (not (procedure? unpure))
I think you want this instead:

(let ((pure (ly:pure-closure-pure-part unpure)))
        (if (not (procedure? pure))
            pure
            (apply pure
                   (append (list (car args) start end) (cdr args)))))

[Neil Puttock, 2011-08-19 22:42:24]

On 18 August 2011 13:44, Mike Solomon <mikesol@ufl.edu> wrote:

> What about pure-container ?

It's all right, I suppose... but what about the unpure part?  After
all, the container's not just about the pure callback.

Cheers,
Neil

[mikesol_ufl.edu, 2011-08-20 08:30:02]

On Aug 20, 2011, at 12:02 AM, Neil Puttock wrote:

> On 18 August 2011 13:44, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> What about pure-container ?
> 

unpure-pure-container ?

Cheers,
MS

[mikesol_ufl.edu, 2011-08-20 20:21:30]

On Aug 20, 2011, at 12:39 AM, n.puttock@gmail.com wrote:

> 
>
http://codereview.appspot.com/4894052/diff/9001/input/regression/pure-closure.ly
> File input/regression/pure-closure.ly (right):
> 
>
http://codereview.appspot.com/4894052/diff/9001/input/regression/pure-closure...
> input/regression/pure-closure.ly:18: #(ly:make-pure-closure
> ly:stem::height '(-3 . 10))
> I'm a bit worried this is too close to an extra-spacing-height override
> to make a good test example.
> 

I'll try to think of something better...if you have any suggestions in the
meantime, they're certainly welcome!

> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc
> File lily/pure-closure.cc (right):
> 
> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode36
> lily/pure-closure.cc:36: return (SCM) SCM_CELL_WORD_1 (smob);
> SCM_PACK (SCM_CELL_WORD_1 (smob));
> 
> (if you want to be strict :)

I have no clue how these SCM functions work, but I'll take your word for it!

> 
> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode43
> lily/pure-closure.cc:43: return (SCM) SCM_CELL_WORD_2 (smob);
> SCM_PACK (SCM_CELL_WORD_2 (smob));
> 

Ditto.

> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode60
> lily/pure-closure.cc:60: SCM_NEWSMOB2 (z, pure_closure_tag, unpure,
> pure);
> SCM_UNPACK (unpure), SCM_UNPACK (pure)
> 

Done.

> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode68
> lily/pure-closure.cc:68: assert (is_pure_closure (pc));
> optimized builds will segfault on invalid args, so you should use
> LY_ASSERT_TYPE () here
> 

Done.

> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode76
> lily/pure-closure.cc:76: assert (is_pure_closure (pc));
> LY_ASSERT_TYPE ()
> 

Done.

> http://codereview.appspot.com/4894052/diff/9001/scm/define-grobs.scm
> File scm/define-grobs.scm (left):
> 
>
http://codereview.appspot.com/4894052/diff/9001/scm/define-grobs.scm#oldcode2655
> scm/define-grobs.scm:2655: (if (not (procedure? unpure))
> I think you want this instead:
> 
> (let ((pure (ly:pure-closure-pure-part unpure)))
>        (if (not (procedure? pure))
>            pure
>            (apply pure
>                   (append (list (car args) start end) (cdr args)))))
> 

I've added a hideous code dup to cover all cases.  This will be attenuated and
removed if I can find the time to move all of LilyPond's pure code over to
unpure-pure-containers.

New patchset up.

Thanks Neil!

Cheers,
MS

[pkx166h, 2011-08-27 08:45:24]

passes make and reg tests

[Neil Puttock, 2011-08-31 20:37:31]

On 2011/08/20 20:21:30, mikesol_ufl.edu wrote:

> I'll try to think of something better...if you have any suggestions in the
> meantime, they're certainly welcome!

Something using a stencil override?

> >
http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode68
> > lily/pure-closure.cc:68: assert (is_pure_closure (pc));
> > optimized builds will segfault on invalid args, so you should use
> > LY_ASSERT_TYPE () here
> > 
> 
> Done.

Not in this patchset.

> 
> >
http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode76
> > lily/pure-closure.cc:76: assert (is_pure_closure (pc));
> > LY_ASSERT_TYPE ()
> > 
> 
> Done.

Not in this patchset.

[Neil Puttock, 2011-08-31 20:41:02]

http://codereview.appspot.com/4894052/diff/19001/lily/system.cc
File lily/system.cc (right):

http://codereview.appspot.com/4894052/diff/19001/lily/system.cc#newcode773
lily/system.cc:773: || is_unpure_pure_container (elts[i]->get_property_data
("Y-extent"))))
move this to pure-relevant?

http://codereview.appspot.com/4894052/diff/19001/lily/unpure-pure-container.cc
File lily/unpure-pure-container.cc (right):

http://codereview.appspot.com/4894052/diff/19001/lily/unpure-pure-container.c...
lily/unpure-pure-container.cc:54: 2, 0, 0, (SCM unpure, SCM pure),
I'd make the pure part optional, if you're going to have grobs where both parts
are identical.

[mikesol_ufl.edu, 2011-09-01 07:23:03]

On Aug 31, 2011, at 10:37 PM, n.puttock@gmail.com wrote:

> On 2011/08/20 20:21:30, mikesol_ufl.edu wrote:
> 
>> I'll try to think of something better...if you have any suggestions in
> the
>> meantime, they're certainly welcome!
> 
> Something using a stencil override?
> 

The problem is that stencils are never considered pure/unpure - they are just
used in grob::pure-height.
I've changed it to Flag instead of Stem to avoid it duplicating the Stem
override of which you speak.

>> >
> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode68
>> > lily/pure-closure.cc:68: assert (is_pure_closure (pc));
>> > optimized builds will segfault on invalid args, so you should use
>> > LY_ASSERT_TYPE () here
>> >
> 
>> Done.
> 
> Not in this patchset.
> 

Fixed.

> 
>> >
> http://codereview.appspot.com/4894052/diff/9001/lily/pure-closure.cc#newcode76
>> > lily/pure-closure.cc:76: assert (is_pure_closure (pc));
>> > LY_ASSERT_TYPE ()
>> >
> 
>> Done.
> 
> Not in this patchset.

Fixed.

[mikesol_ufl.edu, 2011-09-01 07:27:43]

On Aug 31, 2011, at 10:41 PM, n.puttock@gmail.com wrote:

> 
> http://codereview.appspot.com/4894052/diff/19001/lily/system.cc
> File lily/system.cc (right):
> 
> http://codereview.appspot.com/4894052/diff/19001/lily/system.cc#newcode773
> lily/system.cc:773: || is_unpure_pure_container
> (elts[i]->get_property_data ("Y-extent"))))
> move this to pure-relevant?
> 

Good call!  Done.

> http://codereview.appspot.com/4894052/diff/19001/lily/unpure-pure-container.cc
> File lily/unpure-pure-container.cc (right):
> 
>
http://codereview.appspot.com/4894052/diff/19001/lily/unpure-pure-container.c...
> lily/unpure-pure-container.cc:54: 2, 0, 0, (SCM unpure, SCM pure),
> I'd make the pure part optional, if you're going to have grobs where
> both parts are identical.


Done.

Cheers,
MS

[Neil Puttock, 2011-09-01 08:39:24]

http://codereview.appspot.com/4894052/diff/29001/lily/unpure-pure-container.cc
File lily/unpure-pure-container.cc (right):

http://codereview.appspot.com/4894052/diff/29001/lily/unpure-pure-container.c...
lily/unpure-pure-container.cc:35: LY_ASSERT_TYPE (is_unpure_pure_container,
smob, 1);
You sure this works?  It might need registering in function-documentation.cc to
prevent doc build failure.

[mikesol_ufl.edu, 2011-09-01 08:40:19]

On Sep 1, 2011, at 10:39 AM, n.puttock@gmail.com wrote:

> 
> http://codereview.appspot.com/4894052/diff/29001/lily/unpure-pure-container.cc
> File lily/unpure-pure-container.cc (right):
> 
>
http://codereview.appspot.com/4894052/diff/29001/lily/unpure-pure-container.c...
> lily/unpure-pure-container.cc:35: LY_ASSERT_TYPE
> (is_unpure_pure_container, smob, 1);
> You sure this works?  It might need registering in
> function-documentation.cc to prevent doc build failure.
> 

No, but I will certainly check before pushing!

Cheers,
MS

[MikeSol, 2011-09-01 09:47:51]

http://codereview.appspot.com/4894052/diff/29001/lily/unpure-pure-container.cc
File lily/unpure-pure-container.cc (right):

http://codereview.appspot.com/4894052/diff/29001/lily/unpure-pure-container.c...
lily/unpure-pure-container.cc:35: LY_ASSERT_TYPE (is_unpure_pure_container,
smob, 1);
On 2011/09/01 08:39:24, Neil Puttock wrote:
> You sure this works?  It might need registering in function-documentation.cc
to
> prevent doc build failure.

Just a question - in this file, should I call it "unpure_pure_container" or
"unpure pure container"?  I'm leading towards the first (I'll likely run a doc
build with that in a few minutes once my current doc build is done), but I can
change it to the second if that seems more lilypondish.

Cheers,
MS

[MikeSol, 2011-09-01 13:05:35]

Pushed as 69622b49b7a5a9c992e36ef11ba60c1fdd3c34b6.
I made the regtest more unique to unpure-pure-closures so that people reading it
don't mistake it for the same thing as extra-spacing-height.

Have fun with these!

Cheers,
MS