Re: note-name-engraver: add user-defined note names support (issue 221710044 by address@hidden)

[David Kastrup]

address@hidden writes:

> On 2019/02/10 12:23:40, dak wrote:
>> lily/general-scheme.cc:150: LY_DEFINE (ly_rassoc, "ly:rassoc",
>> I am not sure this is wanted (there are also other definitions) since
> it's
>> comparatively slow for large sets.  Can you think of a scheme using a
> hashtable
>> as cache?
>
> Are you sure it’s worth it? Converting the alist into a hash-table
> requires to walk through it item by item, adding each pair to the table.
> Lookups are certainly faster _once_ the table is generated, but that
> still requires an additional step. (I’d suggest reimplementing ice-9’s
> alist->hash-table function in C++, but you’re going to object that
> switching to Guile-v2 will remove any need for that, aren’t you?)

There is alist->hash-table in scm/lily-library.scm and its performance
impact is likely mostly invisible for this kind of application.  It has
the disadvantage of being wrong (and incompatible with the Guile
version) for multiple keys.  One should likely replace hashq-set! with
hashq-create-handle! here.

For printing note names, there is note-name->lily-string exported from
scm/define-music-display-methods.scm which, well, actually does use
rassoc.  Maybe we should just write a hashed-rassoc that uses a weak-key
table for associating alists with hash-tables?  One should not be using
it on destructively modified alists, of course.

Something like

(define rassoc-hashhash (make-weak-key-hashtable))
(define (hashed-rassoc key alist default)
  (let ((hasher (hashq-create-handle! rassoc-hashhash alist #f)))
    (if (not (cdr hasher))
        (set-cdr! hasher (alist->hash-table alist)))
    (hashq-ref (cdr hasher) key default)))

It's not like there aren't a few other uses for it, not too few in the
display-lily machinery.  One would need to think about how to support
the hashq/hashv/hash triple.

So, cough cough.

> I was just mimicking what we already do with ly:assoc-get.  But I know
> now that you’re intending to rewrite it anyway.

There are several loose ends worth getting tied up.  Now the question is
what parts I should be doing for you not to lose interest.

> https://codereview.appspot.com/221710044/diff/60001/lily/note-name-engraver.cc#newcode60
>> lily/note-name-engraver.cc:60: /* FIXME: what if we want to return a
> markup? */
>> You can just call make-concat-markup for concatenating markup.  See
>> lily/lily-imports.cc and lily/include/lily-imports.hh to see how to
> haul that
>> function into C++ (assuming it isn't somewhere already).
>
> Yes, I’ve seen that.  But it seemed cumbersome compared to
>    string s;
>    s += ly_scm2string(other_string);

Doing a single call of
make_markup_concat (scm_list_3 (markup1, markup2, markup3));

does not seem too bad and the export/import is a one-time cost for all
prospective uses.  I am surprised we don't have it already, actually.

> OTOH, this forced me to use Century Schoolbook UTF-8 glyphs for
> accidentals instead of Feta musicglyphs. So a markup may be preferable,
> but I’m still on the fence.

I think on this particular issue, documenting the shortcoming would be a
lot more of a headache than fixing it.

> https://codereview.appspot.com/221710044/

-- 
David Kastrup