[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Uses only unpure-pure containers to articulate unpure-pure relations
From: |
address@hidden |
Subject: |
Re: Uses only unpure-pure containers to articulate unpure-pure relationships. (issue 7377046) |
Date: |
Wed, 27 Feb 2013 23:37:01 +0100 |
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