On 01.09.25 05:25, Michael Paquier wrote:
So, taking a step back, I don't know what would be a good fit for
these duplicates of the "next power" routines upper-bounded on input
when attached to pg_bitutils.h. However, I do see that we can get rid
of pg_log2() and dynahash.h with a consistent interface in
pg_bitutils.h, by reducing my proposal to the introduction of
pg_ceil_log2_32_bound() and pg_ceil_log2_64_bound().
At the end, next_pow2_int64() and next_pow2_int() are a lesser deal to
me, being static to dynahash.c. With that in mind, I am finishing
with the attached. Less ambitious, still it's a nice cleanup IMO.
pg_bitutils.h is aligned with standard compiler intrinsics and in the
long term C23 <stdbit.h>, so we shouldn't add our own custom stuff in
there without considering that bigger picture.
I would agree with what I think you're saying, we can keep these custom
variants with a particular error-checking behavior local to dynahash.c.
Maybe a comment or two to explain this more clearly would be good.
Taking a look at your previous patch with the changes from long to
int64, I think there is something that still doesn't fit.
For example, taking a look at the callers of hash_estimate_size(int64,
Size), they pass either int as the first argument, or in a few cases
long. Looking around inside dynahash.c, do any of these places actually
need the int64 range? These are all just counters, the memory sizes use
Size correctly it seems. Do we want to support more than INT_MAX
elements? I wonder whether the right solution would be to turn the long
uses into int instead. Then you also don't need to deal with two
next_pow2* variants.
