Re: Implement optional music function arguments (issue 5023044)

2011-09-26 Thread dak 


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.scm#newcode52
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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-25 Thread ianhulin44 


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.scm#newcode52
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".

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-23 Thread dak 

Pushed as b4ff85a2416e4b80deb9eef8329cd230ee4dc944 and preceding.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-23 Thread dak 

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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-22 Thread dak 

Pushed as 83055a30e52c14b0fd49d6df3eb1c7af476ecb4b

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-22 Thread pkx166h 

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

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-20 Thread David Kastrup 
carl.d.soren...@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:

afterGrace =
#(define-music-function (parser location main dur grace)
  (ly:music? (ly:duration?) ly:music?)
   (_i "Create @var{grace} note(s) after a @var{main} music expression.
An optional duration between the expressions gives the point of time where the 
grace notes are placed.
")
   (let ((main-length (ly:music-length main))
 (fraction  (ly:parser-lookup parser 'afterGraceFraction)))
 (make-simultaneous-music
  (list
   main
   (make-sequential-music
(list

 (make-music 'SkipMusic
 'duration (or dur
(ly:make-duration
0 0
(* (ly:moment-main-numerator main-length)
   (car fraction))
(* (ly:moment-main-denominator main-length)
   (cdr fraction)
 (make-music 'GraceMusic
 'element grace)))

\new Voice { \afterGrace { c'1 } { c'16 d' }
 \afterGrace { c'1 } 1*7/8 { c'16 d' }
 \afterGrace { c'1} 4 {c'16 d' }
 \afterGrace c'2 {c'16 d'} d'2}
As you can see when comparing with the original in
ly/music-functions-init.ly, the source code is minimally more complex.
Since the default duration needs to be calculated at run time when left
unspecified, we let the optional argument default to #f (defaults are no
longer checked for the correct type, so this works) and splice in the
default calculation with (or dur ...).

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

Neither am I.

> http://codereview.appspot.com/5023044/

-- 
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-20 Thread Carl . D . Sorensen 

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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-20 Thread dak 

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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-19 Thread David Kastrup 
ianhuli...@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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread dak 

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#newcode33

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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread dak 


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#newcode33
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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread pkx166h 

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 # ...
  72:  6* [map # (# # # #
...)]
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$


http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread ianhulin44 

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#newcode33
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.

http://codereview.appspot.com/5023044/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread dak 


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#newcode792
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#newcode801
scm/music-functions.scm:801: "
On 2011/09/15 10:45:11, Reinhold wrote:

Same goes here.

Done

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread dak 


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#newcode762
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#newcode792
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.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread dak 

Reviewers: Reinhold,

Message:
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.



Description:
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.

Please review this at http://codereview.appspot.com/5023044/

Affected files:
  M lily/include/lily-lexer.hh
  M lily/lexer.ll
  M lily/lily-lexer.cc
  M lily/parser.yy
  M scm/document-identifiers.scm
  M scm/lily.scm
  M scm/music-functions.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Implement optional music function arguments (issue 5023044)

2011-09-15 Thread reinhold . kainhofer 

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#newcode762
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#newcode792
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#newcode801
scm/music-functions.scm:801: "
Same goes here.

http://codereview.appspot.com/5023044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel