On Wed, Mar 18, 2026 at 06:34:03PM +0700, John Naylor wrote:
> Seems sensible to me. I only have minor nitpicks:
Thanks for the review of the module.
> +operation for a single byte as well as a range of these, acting as thin
> +wrappers standing on top of pg_saslprep().
>
> It's more natural to say "wrappers around", at least that's what comes to me.
Fixed.
> + if (unlikely(utf8_len == 0))
>
> The exceptional path only has two lines of code, so it's unclear what
> this hint is trying to do. This module isn't run by default anyway
Removed that.
> + MemSet(nulls, false, sizeof(nulls));
>
> Regular "memset" with a 4-byte constant input is easily inline-able by
> the compiler, and I think we should use our homegrown implementation
> only when there is a specific reason for it. (I know there are many
> dozens of uses without a reason already, but...)
Removed that.
> -is($result, 'U+0000|SUCCESS|\x00|\x', "Only nul authorized for all
> valid UTF8 codepoints");
> +is($result, '', "No empty or NULL values for all valid UTF8 codepoints");
>
> I don't quite understand "only nul authorized..." -- I understand the
> explanation in your email, but I having difficulty with the way it's
> phrased here. (Although it'll be moot if we go ahead with 0002)
Yes, still better to keep the state of the tree cleaner at all times,
especially if 0002 gets reverted. I have used a simpler "valid
codepoints returning an empty password".
Applied the result for the module, to have at least the coverage part.
The last piece is refreshed, and attached for now.
--
Michael
