On Sat, 16 May 2026 at 17:08, Timofei Zhakov <[email protected]> wrote:
> On Thu, May 14, 2026 at 7:40 PM Ivan Zhakov <[email protected]> wrote: > >> On Thu, 14 May 2026 at 14:13, Timofei Zhakov <[email protected]> wrote: >> >>> This function counts real printable UTF characters in a string. It >>> currently contains a table of all patterns that is manually checked. I >>> believe it was stolen from elsewhere a long time ago. Before we had >>> utf8proc as a required dependency. >>> >>> I have a few reasons to rewrite it to use the library instead; >>> >>> 1. I'm pretty sure nobody would ever care to update the dataset. On >>> the other hand, utf8proc bundles all available information about the >>> latest Unicode version that is supported on the current platform. >>> >>> 2. There is also a property that defines *display* width, that >>> basically makes symbols like emojis wider than normal characters even >>> on monospace fonts. >>> >>> (For context I want to fix indentation in places throughout our >>> cmdline like the authors in 'svn list -v' that mess up the tables. >>> This is where a function like that will be useful.) >>> >>> 3. Cleanup redundant code. >>> >>> 4. It might be slightly faster to use their dataset because utf8proc >>> only accesses a table in static memory twice (for address and then >>> retrieves properties) instead of binary searching and checking all >>> ranges. Maybe it's slower though idk. >>> >>> Thoughts? >>> >>> Sounds good to me. >> >> Regarding potential performance regression: is it something we can >> measure? As far as I understand svn_utf_cstring_utf8_width() is not used >> for performance critical code, but it would be nice to know if there is >> significant performance regression anyway. >> >> > Well, I guess it was a bad argument because the utf8proc version looks > well optimised anyway and we already use in multiple places. > > I'm done with the first version of a patch to make this switch. Please > find it in the attachments. I also added a unit test to confirm. > > The patch looks good to me. A few minor suggestions: 1. I suggest to split patch in separate commits: one with test, another with actual code change and adjusting test expectation. This would help to compare the difference. 2. I suggest to add test case for Byte Order Mark (BOM): const char *bom = "\xEF\xBB\xBF" "abc"; 3. I agree that call to svn_utf__cstring_is_valid() can be removed since utf8proc_iterate() checks for invalid UTF-8 sequence. Thanks! I think since this function depends on utf8proc it should be implemented in > utf8proc.c file. Hence utf_width.c can be removed entirely. I guess the > main reason it was in a separate file was to keep the dataset away from > other code. > > By the way, I also found that before traversing the string, this function > calls svn_utf__cstring_is_valid() to check validity. I > think utf8proc_iterate() does the same check, but I really am not > sure. svn_utf__cstring_is_valid() skips all first octets with values >0x80. > Then it basically does the same work utf8proc_iterate() would. > > -- Ivan Zhakov
