Re: Make music functions callable from Scheme (issue 244840043 by address@hidden)

[dak]

This mainly addresses the points of Keith's critique where feasible.
Several proposals were inadvisable or based on wrong assumptions: I've
put in extensive documentation in order to make this more obvious.

Apart from slight code arrangement and lots of documentation, I also
removed functionality from the Fluid class that has
shoot-yourself-in-the-foot potential, and I fixed a slight error in the
fallback for printing of notenames without accessible parser.


https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely#newcode68
Documentation/changes.tely:68: as if they were proper Scheme functions.
Argument checking will
On 2015/06/08 09:17:27, dak wrote:
> On 2015/06/07 22:49:05, Keith wrote:
> > On 2015/06/07 22:16:00, Keith wrote:
> > > Skip "as if they were.." if they really are proper Scheme
functions.
> >
> > I guess the result of
> >  (define-music-function ... )
> > with all its type-checking and references to the source line-numbers
is
> > something more than a simple Scheme function, so I see why you say
"as if
> they
> > were"

> Well, when called from Scheme there is not much in the line of
"references to
> the source line-numbers".  The result of define-music-function would
now meet
> the predicate procedure?, had procedure-properties so it is sort of a
function.
> But it is actually a GUILE data structure which has additional
function-call
> semantics.  And calling procedure-environment on it fails (no idea how
this
> would behave in GUILE 2 though: it seems like allowing capture of
a procedure
> environment could be an interesting extension).  The C++ equivalent
would be
> something like being derived from a function class (actual C++
functions are not
> of a type that can be used as base class but there are some comparable
> constructs).

> Maybe I should not use "proper" but "genuine" here.  I don't think
GUILE has an
> established term for this we could use.

Done.

https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh
File lily/include/fluid.hh (right):

https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#newcode24
lily/include/fluid.hh:24:
On 2015/06/08 09:32:06, dak wrote:
> On 2015/06/07 22:16:00, Keith wrote:
> > "storage of Scheme fluids" so that people know you mean the
Lisp/Scheme
> concept
> > and not the normal meaning of 'fluid'.

> Probably rather "GUILE fluids" since Scheme in general does not have
them.
> Scheme srfi-39 provides "parameters" which are somewhat similar.

Done.

https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#newcode36
lily/include/fluid.hh:36: // where the garbage collector will be able to
find it.
On 2015/06/07 22:16:02, Keith wrote:
> The passive voice leaves things ambiguous.  I think "declare only
local Fluid
> variables, not statics, not arrays of Fluids, so that the Fluid object
is in
> automatic storage where the Guile garbage collector can find it."

Acknowledged.  I replaced the whole paragraph instead.

https://codereview.appspot.com/244840043/diff/80001/lily/include/fluid.hh#newcode44
lily/include/fluid.hh:44: SCM value_;
On 2015/06/08 09:32:06, dak wrote:
> On 2015/06/07 22:16:02, Keith wrote:
> > // the cache

> If that comment would clarify matters, it would seem like I should be
calling
> the field "cache_" in the first place.  Is fluid_/cache_ better than
> fluid_/value_?  I think the latter captures the distinction between
container
> and content better.

Acknowledged.  Added a more verbose comment.

https://codereview.appspot.com/244840043/diff/80001/lily/include/music-function.hh
File lily/include/music-function.hh (right):

https://codereview.appspot.com/244840043/diff/80001/lily/include/music-function.hh#newcode34
lily/include/music-function.hh:34: SCM call (SCM args);
On 2015/06/07 22:16:02, Keith wrote:
> Wouldn't this be more conventional as a static member function ?
>    static SCM call( SCM music_function_smob, SCM args_list);

LY_DECLARE_SMOB_PROC works with non-static member function pointers.
The reasons for that were reviewed separately when it was introduced.
Not relevant for reviewing this issue.

https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc
File lily/music-function.cc (right):

https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#newcode83
lily/music-function.cc:83: skipping = true;
On 2015/06/08 04:36:54, Keith wrote:
> Here we failed to type-match an optional argument, and will fill the
slot with
> the default value on line 88, so why set the 'skipping' flag ?  If
there are two
> optional parameters in a row, the 'skipping' flag would seem to force
the next
> entry in the signature to be filled with its default value as well.

I've added extensive documentation and references to make clear this is
correct.

https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#newcode88
lily/music-function.cc:88: args = scm_cons (with_loc (scm_cdr (pred),
location), args);
On 2015/06/08 06:09:31, dak wrote:
> On 2015/06/08 04:36:54, Keith wrote:
> > For an optional argument with no default value, it seems we would
want #f, but
> > scm_cdr will return the empty list.

> Signatures are not stored in the original form.  They are slightly
processed in
> order to make their use faster.  scm_cdr will return #f since (number?
. #f) is
> what will be stored for a predicate written as (number?) while
(number? . 7) is
> what will be stored for (number? 7).

> Could be worth mentioning.

Done.

https://codereview.appspot.com/244840043/