Re: Add the command \offset to LilyPond (issue 8647044)

[dak]

On 2013/10/06 01:15:12, david.nalesnik wrote:

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly
> File input/regression/offsets.ly (right):


https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#newcode5
> input/regression/offsets.ly:5: the @code{\\offset} command.  These
properties
> are limited to immutable
> On 2013/04/23 20:24:57, dak wrote:
> > What does "immutable" mean here?

> Hopefully this rewrite is less opaque.


https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#newcode8
> input/regression/offsets.ly:8: in its default appearance.  The command
is used
> both as a tweak and an
> On 2013/04/23 20:24:57, dak wrote:
> > "demonstrated as a tweak and as an override".  The double "as" is
important,
> and
> > I'd remove "both" since otherwise the impression arises that it is
both at the
> > same time.

> Done.

> https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm
> File scm/c++.scm (right):

> https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm#newcode30
> scm/c++.scm:30: (every number-pair? x)))
> On 2013/10/04 01:17:08, david.nalesnik wrote:
> > On 2013/04/23 20:24:57, dak wrote:
> > > Isn't it dangerous to call "every" on something that is not
necessarily a
> > list?
> > > Like (cons 2 3)?
> >
> > I would think so... However,
> > (every number-pair? (cons 2 3))
> > returns #f

> Fixed.  No more reliance on undefined behavior.


https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
> File scm/music-functions.scm (right):


https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newcode2103
> scm/music-functions.scm:2103: ; head of the alist.  We reverse the
alist so our
> search will return
> Thank you for your explanations!

> On 2013/10/04 05:59:15, dak wrote:

> > It is probably more interesting how the function offsetter is then
ended:
> >
> > (define (offsetter property offsets)
> >   (define (self grob) .............)
> >   self)

> This is what stumped me.  I was attempting to return (self grob).
> --------
> There is a major problem with this patch set which I don't know how to
address.

> The examples below represent my efforts to test the effects of
multiple
> applications of \offset.  You can see that some accumulation is
possible.
> However, the last two examples (commented out) will cause a crash.
All I can
> think of is that this relates to the multiple instances of "self" in
the
> properties alist.  I've verified that the instances aren't identical
despite
> having the same name, so I don't understand why there should be a
problem.

I suppose the problem is with
+(define (find-value-to-offset prop self alist)
+  "Return the first value of the property @var{prop} in the property
alist @var
+that is not @var{self}."
+  (let loop ((ls alist))
+    (if (null? ls)
+        #f
+        (if (eq? prop (car (first ls)))
+            (if (eq? self (cdr (first ls)))
+                (loop (cdr ls))
+                (cdr (first ls)))
+            (loop (cdr ls))))))

First: try using GUILE srfi-1 functions for that kind of thing: they are
more readable than explicit loops.

And nice to have accurate documentation: this tells us exactly what is
wrong:

+  "Return the first value of the property @var{prop} in the property
alist @var
+that is not @var{self}."

But what you need here is "return the first value of the property
@var{prop} in the property
alist @var{alist} @em{after} having found @var{self}."

You _always_ have to _first_ pass self before looking for anything else.
 If you find a valid property _before_ finding self, ignore it and
continue looking.  Otherwise you end up in a mutual recursion.

If you don't find self at all, its head-scratching time: after all, who
called you then?  I lean toward throwing an error: continuation
strategies might be fragile.

https://codereview.appspot.com/8647044/