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