On 27 févr. 2013, at 19:06, d...@gnu.org 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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel