Re: Uses only unpure-pure containers to articulate unpure-pure relationships. (issue 7377046)

[address@hidden]

On 27 févr. 2013, at 19:06, address@hidden wrote:

> 
> https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-text-spanner.ly
> File input/regression/scheme-text-spanner.ly (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/input/regression/scheme-text-spanner.ly#newcode129
> input/regression/scheme-text-spanner.ly:129:
> side-position-interface::y-aligned-side)
> I really don't understand why you ask on the developer list about
> gratuitous prefix changes because of a different implementation language
> when you propose such changes right afterwards.
> 

In my e-mail, I stated:
"I'd prefer if all native Scheme functions did not have the ly: prefix - it 
helps to know what things are where."
side-position-interface::y-aligned-side above is a native Scheme function that 
does not have the ly: prefix.

> https://codereview.appspot.com/7377046/diff/17001/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/lily/axis-group-interface.cc#newcode261
> lily/axis-group-interface.cc:261: if (!g->is_live ())
> Shouldn't you check for liveness before anything else?

The call to check the cross-staff property above could trigger the suicide.
In general, it is better to access properties first, suicide later.

> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob-property.cc
> File lily/grob-property.cc (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob-property.cc#newcode345
> lily/grob-property.cc:345: if (is_simple_closure (unpure))
> This is not related to this patch and review, but can anybody explain
> why this simple_closure $#!+ can't be replaced by a callable closure
> created using a smob type made callable via scm_set_smob_apply?  Is it
> because of the delayed arg?  Can't we deal with that by using a fluid
> (in Scheme, a parameter) in evaluate_with_simple_closure?  These pseudo
> "simple" closures really make my head ache, and being able to throw all
> of them out would be quite a boon.

Agreed.

> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob-scheme.cc
> File lily/grob-scheme.cc (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob-scheme.cc#newcode454
> lily/grob-scheme.cc:454: SCM_ASSERT_TYPE (ly_is_procedure (proc) ||
> is_unpure_pure_container (proc), proc, SCM_ARG2, __FUNCTION__,
> "procedure or unpure pure container");
> Possibly worth a named type predicate of its own?

Possibly.  Could be in a separate patch.

> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc
> File lily/grob.cc (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/lily/grob.cc#newcode866
> lily/grob.cc:866: if (to_boolean (scm_object_property
> (me->get_property_data ("stencil"), ly_symbol2scm ("ly:stencil?"))))
> Where is this object property being set?

In define-grobs.scm.

> 
> https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm
> File scm/output-lib.scm (right):
> 
> https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm#newcode61
> scm/output-lib.scm:61: (define-public
> (grob::wrap-in-unpure-pure-container fn)
> After issue 3200 passed, this is just ly:make-unpure-pure-container.
> Remove, change all its callers.

Ok. I'll try to remember for this patch set.

> 
> https://codereview.appspot.com/7377046/diff/17001/scm/output-lib.scm#newcode89
> scm/output-lib.scm:89: (define-public
> grob::always-vertical-skylines-from-stencil
> I don't think that these are worth a definition of their own.  Better
> expand them inline.  An unpure-pure container is cheap enough that
> having separate containers for each use is not really a problem.
> 

They reappear several times.  It is a useful heuristic so that if I need to 
change how this works, I only need to change it in one place instead of several.

Thanks for the review!

Cheers,
MS