Re: Wrong type in part-combiner.scm

[David Kastrup]

Aaron Hill <lilypond@hillvisions.com> writes:

> On 2020-09-22 5:54 am, David Kastrup wrote:
>> The whole code in the find-non-empty loop, apart from this error, is
>> garbage.  Here is a bit more code and analysis:
>> (define-public (add-quotable name mus)
>>   (let* ((tab (eval 'musicQuotes (current-module)))
>>          (voicename (get-next-unique-voice-name))
>>          ;; recording-group-emulate returns an assoc list
>> (reversed!), so
>>          ;; hand it a proper unique context name and extract that key:
>>          (ctx-spec (context-spec-music mus 'Voice voicename))
>>          (listener (ly:parser-lookup 'partCombineListener))
>>          (context-list (reverse (recording-group-emulate ctx-spec
>> listener)))
>>          (raw-voice (assoc voicename context-list))
>>          (quote-contents (if (pair? raw-voice) (cdr raw-voice) '())))
>>     ;; If the context-specced quoted music does not contain
>> anything, try to
>>     ;; use the first child, i.e. the next in context-list after
>> voicename
>>     ;; That's the case e.g. for \addQuote "x" \relative c \new Voice
>> {...}
>>     (if (null? quote-contents)
>>         (let find-non-empty ((current-tail (member raw-voice
>> context-list)))
>>           ;; if voice has contents, use them, otherwise check next ctx
>>           (cond ((null? current-tail) #f)
>>                 ((and (pair? (car current-tail))
>>                       (pair? (cdar current-tail)))
>>                  (set! quote-contents (cdar current-tail)))
>>                 (else (find-non-empty (cdr current-tail))))))
>>     (if (not (null? quote-contents))
>>         (hash-set! tab name (list->vector (reverse! quote-contents
>> '())))
>>         (ly:music-warning mus (ly:format (_ "quoted music `~a' is
>> empty") name)))))
>> So raw-voice is an assoc into context-list.  This assoc is then 
>> searched
>> for in the find-non-empty loop.  For each found instance, a test is
>> made.  If that test is not successful, the search is repeated for more
>> instances.  However, since all found instances are _guaranteed_ to be
>> the same (since assoc is only called once, and then member searches for
>> the same key/value pair rather than just the same key), the test is
>> guaranteed to deliver the same result for each found instance.
>> This is just crap.  Presumably the idea was to search for the same
>> key
>> but potentially different values?
>
> I don't think this code matches multiple times.  The call to member
> only occurs upon the first invocation of find-non-empty.

Ouch.  I need to recheck my Scheme-fu.

> Each subsequent time, the current-tail parameter gets the value of
> (cdr current-tail) which is simply iterating the list until running
> out of items.

OK.

> The problem with raw-voice being #f, though, is an issue.  It is
> highly unlikely to be found in the context-list, so member produces
> another #f that ultimately leads to the failing (cdr #f).  At first, I
> would wonder if raw-voice should never be #f but then I see code
> checking (pair? raw-voice), so it must be valid edge case.
>
> What I cannot tell is whether the entire context-list is to be
> searched when raw-voice is #f or whether the find-non-empty logic
> should be skipped altogether.
>
> So, if we want to fall back to searching the entire list, do this:
>
> ;;;;
>   (let find-non-empty ((current-tail (or (member raw-voice context-list)
>                                          context-list)))
> ;;;;
>
> Or if we want to skip the searching, do this:
>
> ;;;;
>   (if (and (null? quote-contents) (pair? raw-voice))
> ;;;;
>
> Absolutely no idea which is correct.  Sorry.  :/

Dan?  I think this issue started occuring after a change of yours, so do
you have an idea about the meaning of raw-voice becoming #f here?

-- 
David Kastrup