Re: [unigbrk 2/2] unigbrk: New modules for grapheme clusters.

[Ben Pfaff]

Bruno Haible <address@hidden> writes:

> - The functions u#_grapheme_len and u#_grapheme_next are
>   redundant with each other:
>     u8_grapheme_len (s, n) ::=
>     (n == 0 ? 0 : (u8_grapheme_next (s, s + n) ? : end) - s).
>   I would not offer two APIs that are _that_ similar for
>   doing the same thing. (The functions in unistr.h do
>   have similar redundancies, but there the rationale is
>   similarity with the ISO C wchar_t API.) And the functions
>   returning a pointer are more symmetric when you consider
>   forward and backward iteration together.
>
>   So my slight preference would be to remove the
>   u#_grapheme_len functions and keep u#_grapheme_next instead.

OK, that's fine.  I'll push a change that removes the *_len
functions soon.

> - Naming of functions: Now some functions have
>   _grapheme_cluster_ in their name, some have _grapheme_ only.
>   How about using either _grapheme_ consistently or
>   _grapheme_cluster_ consistently?
>
>   It's not necessary to rename the files and modules, though.

I prefer _grapheme_ over _grapheme_cluster_ for use in function
names, because it is shorter but as far as I can tell it is not
ambiguous with _grapheme_cluster_.

Only one function has grapheme_cluster in its name.  I'll push a
change that renames it soon.

> - Code duplication: In lib/ (not in tests/) it is often possible to
>   use the same algorithm for the 8-bit unit, 16-bit unit, and 32-bit unit
>   cases. It helps future maintenance if the same algorithm is present
>   in 1 file and not 3 files. Would you disagree if I use the de-duplication
>   technique (with a parametrized .h file) on your new files?
>
>   In the tests, unfortunately, this is often not possible.

This sounds fine with me, if you want to do it.

> - Typo in unigbrk.h: 's/ ben / been /'

I pushed a fix.  I'll email it out with the others, too, in a few
minutes.

Thank you for all the feedback!
-- 
Ben Pfaff 
http://benpfaff.org