On 15.10.22 01:00, Jeff Davis wrote:
On Thu, 2022-10-13 at 10:57 +0200, Peter Eisentraut wrote:
It's a bit confusing that arguments must be NUL-terminated, but the
length is still specified. Maybe another sentence to explain that
would
be helpful.
Added a comment. It was a little frustrating to get a perfectly clean
API, because the callers do some buffer manipulation and optimizations
of their own. I think this is an improvement, but suggestions welcome.
If win32 is used with UTF-8 and wcscoll, it ends up allocating some
extra stack space for the temporary buffers, whereas previously it used
the buffers on the stack of varstr_cmp(). I'm not sure if that's a
problem or not.
Refactoring the common code from varstr_cmp() and varstrfastcmp_locale()
looks like a clear win.
But I think putting the Windows UTF-8 code (win32_utf8_wcscoll()) from
varstr_cmp() into pg_strcoll() might create future complications.
Normally, it would be up to varstr_sortsupport() to set up a
Windows/UTF-8 specific comparator function, but it says there it's not
implemented. But someone who wanted to implement that would have to
lift it back out of pg_strcoll, or rearrange the code in some other way.
It's not a clear win, I think. Perhaps you have some follow-up work
planned, in which case it might be better to consider this in more
context. Otherwise, I'd be tempted to leave it where it was in
varstr_cmp(). (It could still be a separate function, to keep
varstr_cmp() itself a bit smaller.)
