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

[v . villenave]

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?)

BTW, you gotta love the FIXME in Guile’s hash-tables source code:
https://git.savannah.gnu.org/cgit/guile.git/tree/libguile/hashtab.c#n722


https://codereview.appspot.com/221710044/diff/60001/lily/general-scheme.cc#newcode156
> lily/general-scheme.cc:156: LY_ASSERT_TYPE (ly_cheap_is_list, alist,
2);
> Using a predicate prone to make later code crash is not a good idea.
Just use
> ly_is_list instead.

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


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);
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.

V.

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