Implement optional music function arguments

Implement optional music function arguments

This allows, say, to define a substitute for \relative that has an
optional pitch argument defaulting to f rather than c.

pitch = #(define-scheme-function (parser location pitch)
	  (ly:pitch?) pitch)
myrelative = #(define-music-function (parser location pitch music)
	       ((ly:pitch? #{ \pitch f #}) ly:music?)
	       #{ \relative $pitch $music #})
\relative c' {c' d e f g a b c}
\relative {c' d e f g a b c}
\myrelative c' {c' d e f g a b c}
\myrelative {c' d e f g a b c}

The first uploaded patch is a separate commit with the following description:

lexer.ll: Allow push_extra_token to take a Scheme value as well.

[Reinhold, 2011-09-15 10:45:11]

Regtest is missing (doesn't need to be a useful example, it just needs to break
if that functionality ever breaks!)

Also, does this work for cases like
  \relative c' c

Also, I suppose things like 
  \myfunction [optional-pitch] pitch music 
does not work due to the lookahead not looking too far, right?

http://codereview.appspot.com/5023044/diff/2001/lily/parser.yy
File lily/parser.yy (right):

http://codereview.appspot.com/5023044/diff/2001/lily/parser.yy#newcode1184
lily/parser.yy:1184: | EXPECT_MARKUP EXPECT_OPTIONAL function_arglist
function_markup_argument {
Can't we shorten this long list of alternatives somehow?

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm
File scm/music-functions.scm (right):

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:762: "Helper macro for `ly:make-music-function'.
It's also a helper for ly:make-scheme-function...

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:792: "
Here you should add a description how optional arguments are given! In
particular, the argX-type? is no longer valid in general.

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:801: "
Same goes here.

[dak, 2011-09-15 13:23:41]

On 2011/09/15 10:45:11, Reinhold wrote:
> Also, does this work for cases like
>   \relative c' c

Yes, it does.  Parameters following non-present optional parameters are more
restricted than those following present optional parameters.

While you can't write \myrelative c' instead of \myrelative { c' }, \myrelative
c' c instead of \myrelative c' { c } works just fine since you can't confuse it
with \myrelative { c' } c.

> Also, I suppose things like 
>   \myfunction [optional-pitch] pitch music 
> does not work due to the lookahead not looking too far, right?

Correct.  One could try to squeeze appropriate patterns into the syntax as well,
but the current Scheme requires already O(n^2) rules, and extending the patterns
to cover the generalizations of your example would require O(n^3) rules.  Too
much pain for the gain.

[dak, 2011-09-15 13:23:57]

http://codereview.appspot.com/5023044/diff/2001/lily/parser.yy
File lily/parser.yy (right):

http://codereview.appspot.com/5023044/diff/2001/lily/parser.yy#newcode1184
lily/parser.yy:1184: | EXPECT_MARKUP EXPECT_OPTIONAL function_arglist
function_markup_argument {
On 2011/09/15 10:45:11, Reinhold wrote:
> Can't we shorten this long list of alternatives somehow?

Not really.  You need to combine each argument type x with a list of argument
types different from x.  So each of the n(n-1) combinations has no constituents
shared with the other constituents.  You could factor out parts of that list. 
Then you have n different factors for which you need a good fitting name each. 
And the amount of rule lines increases even more, and there is no real advantage
in readability because there is not much of a system except you need to cover
the O(n^2) cases.

This is definitely the ugly part of the patch.  It will not significantly affect
performance, thanks to Bison and LALR(1), but it is a headache to read.  And I
see no way around it.

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm
File scm/music-functions.scm (right):

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:762: "Helper macro for `ly:make-music-function'.
On 2011/09/15 10:45:11, Reinhold wrote:
> It's also a helper for ly:make-scheme-function...

There is no ly:make-scheme-function, so it can't help it.  All syntactic
functions are created with ly:make-music-function.

While I have some leaning towards define-lily-function (as it takes Lily
arguments), this is not actually anything introduced by this patch (rather part
of the define-scheme-function patch series).  So if you want different names/doc
strings, you should file them as a separate issue.

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:792: "
On 2011/09/15 10:45:11, Reinhold wrote:
> Here you should add a description how optional arguments are given! In
> particular, the argX-type? is no longer valid in general.

Yes to the first, not quite to the second.  The argX-type? remains valid. 
Anybody have a good suggestion what to name the parameters in the DOC string? 
They can be either predicate? or (predicate? default) for an optional parameter
taking a default value.  The default is evaluated at definition time, so using
#{...#} and similar in the defaulsts does not impact execution time.

[dak, 2011-09-15 15:18:08]

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm
File scm/music-functions.scm (right):

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:792: "
On 2011/09/15 10:45:11, Reinhold wrote:
> Here you should add a description how optional arguments are given! In
> particular, the argX-type? is no longer valid in general.

Done

http://codereview.appspot.com/5023044/diff/2001/scm/music-functions.scm#newco...
scm/music-functions.scm:801: "
On 2011/09/15 10:45:11, Reinhold wrote:
> Same goes here.
Done

[Ian Hulin (gmail), 2011-09-15 19:44:41]

Looks pretty cool, apart from some involved Scheme which I couldn't really
unravel totally (see below).
  
Will this patch allow us to get rid of the abomination of  \afterGraceFraction
by recasting \afterGrace to have an optional parameter 
\afterGrace {note} {gracenotes} [spacing-fraction]
e.g.
c1 \afterGrace d1 { c16[ d] } c1 ;; use default spacing
c1 \afterGrace d1 { c16[ d] } 15/16 c1
;or
c1 \afterGrace d1 { c16[ d] } 1/2 c1
;instead of 
#(define afterGraceFraction (cons 15 16))
c1 \afterGrace d1 { c16[ d] } c1
;or
#(define afterGraceFraction (cons 1 2))
c1 \afterGrace d1 { c16[ d] } c1

Keep up the good work, and thanks.
Ian

http://codereview.appspot.com/5023044/diff/9001/scm/document-identifiers.scm
File scm/document-identifiers.scm (right):

http://codereview.appspot.com/5023044/diff/9001/scm/document-identifiers.scm#...
scm/document-identifiers.scm:33: (format $f "(~a)" (type-name pred)))))
What does $f do in the format destination here, where's it declared? The Guile
documentation mentions #f returning the output as a string, otherwise it's a
port. So what port does $f represent?
Does this tie up with the (define-syntax-function) changes in
music-functions.scm?  Is $f definitely not a typo?
Comment this section, please, David.

[pkx166h, 2011-09-15 20:17:00]

Fails make
--snip--
Backtrace:
In unknown file:
   ?:  0* [primitive-load-path "documentation-generate.scm"]
In
/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/documentation-generate.scm:
  72:  1* [display ...
  73:  2*  [identifiers-doc-string]
In
/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm:
  64:  3   [format #f "@table @asis
~a
@end table
" ...
  69:  4*   [string-join ...
  70:  5*    [filter #<procedure identity (x)> ...
  72:  6*     [map #<procedure document-object (obj-pair)> (# # # # ...)]
In unknown file:
   ?:  7*     [document-object (acciaccatura . #)]
In
/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm:
 57:  8*     (cond (# #) (else #f))
  59:  9      [document-music-function (acciaccatura . #)]
  21: 10      (let* # #)

/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm:21:3:
In procedure memoization in expression (let* (# # # ...) (format #f "@item
@code{~a} ~a ~a~a
@funindex ~a
~a
" ...)):
/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm:21:3:
In file
"/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm",
line 29: Bad binding (type-names (map (lambda (pred) (if (pair? pred) (format #f
"[~a]" (type-name (car pred))) (format $f "(~a)" (type-name pred))))) sign) in
expression (let* ((name-sym (car music-func-pair)) (music-func (cdr
music-func-pair)) (func (ly:music-function-extract music-func)) (arg-names (map
symbol->string (cddr (cadr (procedure-source func))))) (doc
(procedure-documentation func)) (sign (object-property func (quote
music-function-signature))) (type-names (map (lambda (pred) (if (pair? pred)
(format #f "[~a]" #) (format $f "(~a)" #)))) sign) (signature-str (string-join
(map (lambda (x) (format #f "@var{~a} ~a" # #)) (zip arg-names (cdr
type-names)))))) (format #f "@item @code{~a} ~a ~a~a
@funindex ~a
~a
" name-sym (car type-names) (if (equal? "" signature-str) "" " - ")
signature-str name-sym (if doc doc (begin (ly:warning "music function `~a' not
documented." name-sym) "(undocumented; fixme)")))).
make[1]: *** [out/internals.texi] Error 1
make[1]: Leaving directory `/home/jlowe/lilypond-git/build/Documentation'
make: *** [all] Error 2
jlowe@jlowe-lilybuntu2:~/lilypond-git/build$

[dak, 2011-09-15 21:29:20]

http://codereview.appspot.com/5023044/diff/9001/scm/document-identifiers.scm
File scm/document-identifiers.scm (right):

http://codereview.appspot.com/5023044/diff/9001/scm/document-identifiers.scm#...
scm/document-identifiers.scm:33: (format $f "(~a)" (type-name pred)))))
On 2011/09/15 19:44:41, Ian Hulin (gmail) wrote:
> What does $f do in the format destination here, where's it declared? The Guile
> documentation mentions #f returning the output as a string, otherwise it's a
> port. So what port does $f represent?
> Does this tie up with the (define-syntax-function) changes in
> music-functions.scm?  Is $f definitely not a typo?
> Comment this section, please, David.

Typo.  I have not rebuilt the documentation.  Thanks for catching this.

[dak, 2011-09-15 23:12:31]

On 2011/09/15 21:29:20, dak wrote:
> http://codereview.appspot.com/5023044/diff/9001/scm/document-identifiers.scm
> File scm/document-identifiers.scm (right):
> 
>
http://codereview.appspot.com/5023044/diff/9001/scm/document-identifiers.scm#...
> scm/document-identifiers.scm:33: (format $f "(~a)" (type-name pred)))))
> On 2011/09/15 19:44:41, Ian Hulin (gmail) wrote:
> > What does $f do in the format destination here, where's it declared? The
Guile
> > documentation mentions #f returning the output as a string, otherwise it's a
> > port. So what port does $f represent?
> > Does this tie up with the (define-syntax-function) changes in
> > music-functions.scm?  Is $f definitely not a typo?
> > Comment this section, please, David.
> 
> Typo.  I have not rebuilt the documentation.  Thanks for catching this.

Ok, this should now be better.

[dak, 2011-09-19 12:36:52]

ianhulin44@gmail.com writes:

> Looks pretty cool, apart from some involved Scheme which I couldn't
> really unravel totally (see below).
>
> Will this patch allow us to get rid of the abomination of
> \afterGraceFraction by recasting \afterGrace to have an optional
> parameter
> \afterGrace {note} {gracenotes} [spacing-fraction]

You'd rather have to make this

\afterGrace [spacing-fraction] {note} {gracenotes}

or similar.  I am not too clear on just what would work as an argument
type for spacing-fraction.  Scheme as in #'(15 . 16) or a duration as in
1*15/16 (ugh, what with the 1?).

Personally, I'd likely just go for
\afterGrace \times 15/16 {note} {gracenotes}

\afterGrace can easily check whether it gets scaled music and unpack
it.  If you really want to write scaled music, you can do so as
\afterGrace { \times 15/16 ...
in which case you get a sequential music event as topmost expression
which you leave alone.

This will turn out weird if you did
somemacro = \times 2/3 { .... }
and then use
\afterGrace \somemacro ...
because then you'll likely have no idea what happens.  Or if you do
\afterGrace on tuplets without bothering to enclose them in braces.

This would work fine without involving any functionality from this patch
series.

Or you do
\afterGrace [spacing-duration] ...

instead and forget about the fraction (durations can be fractions after
all).

-- 
David Kastrup

[dak, 2011-09-20 23:24:56]

Next round.  Getting past shift/reduce conflicts required adding precendences to
every terminal token that can start the last non-optional argument.

But the resulting grammar is O(n): adding more elements to it is quite
straightforward.

[Carl, 2011-09-20 23:43:29]

I'd be lying if I said I understood everything going on here, but I think I get
the gist.

I like moving this way!

I like the approach of simplifying things.

I like having optional predicates, and optional predicates with defaults.

I will trust you that it is O(n) and that all the shift-reduce conflicts have
been resolved.

I'm not a parser expert, so it doesn't mean much coming from me, but I think
this looks good.

[dak, 2011-09-21 06:27:59]

Carl.D.Sorensen@gmail.com writes:

> I'd be lying if I said I understood everything going on here, but I
> think I get the gist.

Same here.

> I like moving this way!
>
> I like the approach of simplifying things.
>
> I like having optional predicates, and optional predicates with
> defaults.
>
> I will trust you that it is O(n) and that all the shift-reduce conflicts
> have been resolved.

No need to trust: I was talking about O(n) concerning the number of
rules.  Since there are no longer rules with EXPECT_A EXPECT_B
_combinations_, adding new types will add a number of rules to the
grammar that is proportional to the number of new types.

The cost is that one needs to either
a) have a precedence rule for _every_ terminal symbol that may start a
   function argument
b) be prepared to ignore a large number of shift/reduce conflicts.

There are also two restrictions that may be arbitrary:
a) if you leave out an optional argument, all immediately following
optional arguments are also skipped.  The reason is that I don't want a
puzzle game for filling optional arguments into a list of argument types
A A B C A C .  If you get arguments A C in the input, where will they
end up?
b) the last argument needs to be non-optional.  Otherwise a call ending
with five optional arguments can look five syntactic arguments ahead in
the input before deciding it does not want any.

I don't actually think that the parser can deal with this kind of
lookahead, so this may cut down expectations to a more reasonable level.

As to performance: that is more or less O(n*l) where n is input size and
l is the average lookahead piling up.  Lookahead is pretty much limited:
this mostly assigns arguments left to right, skipping optional argument
lists once the first input does not fit.

I wanted the code to be simpler but that is really hard.  Anyway, here
is an application:

[pkx166h, 2011-09-22 19:37:36]

Not sure what tracker (if any) issue this is but it now passes make and reg
tests.

[dak, 2011-09-22 20:11:52]

Pushed as 83055a30e52c14b0fd49d6df3eb1c7af476ecb4b

[dak, 2011-09-23 22:19:22]

Regtests and docs are there.  The last upload, however, seems to have influences
from a few unrelated commits.  Rietveld is too complicated for me.

On the plus side, a working version (compiles the docs as far as I can tell) has
been pushed to dev/staging.

[dak, 2011-09-24 05:25:59]

Pushed as b4ff85a2416e4b80deb9eef8329cd230ee4dc944 and preceding.

[Ian Hulin (gmail), 2011-09-25 22:25:35]

http://codereview.appspot.com/5023044/diff/22001/scm/ly-syntax-constructors.scm
File scm/ly-syntax-constructors.scm (right):

http://codereview.appspot.com/5023044/diff/22001/scm/ly-syntax-constructors.s...
scm/ly-syntax-constructors.scm:52: (format #f (_ "~a function can't return ~a")
(_ "~a function cannot return ~a")
Sounds more official and serious and less chatty than "can't".

[dak, 2011-09-26 07:46:40]

http://codereview.appspot.com/5023044/diff/22001/scm/ly-syntax-constructors.scm
File scm/ly-syntax-constructors.scm (right):

http://codereview.appspot.com/5023044/diff/22001/scm/ly-syntax-constructors.s...
scm/ly-syntax-constructors.scm:52: (format #f (_ "~a function can't return ~a")
On 2011/09/25 22:25:35, Ian Hulin (gmail) wrote:
> (_ "~a function cannot return ~a")
> Sounds more official and serious and less chatty than "can't". 

Picking German as one of the more complete catalogs:

$ git grep -c "can't" po/de.po
po/de.po:12
$ git grep -c "cannot" po/de.po
po/de.po:63
$ git grep -c "can not" po/de.po
po/de.po:2

Does not seem like that policy is kept perfectly consistent.  On the other hand,
I have to admit being surprised that we use the predominantly British spelling
here while using "color" exclusively and preferring "neighbor" over "neighbour"
3:1 and using "centre" almost only in the translations.

I'll do the change soonish.