On Sat, Apr 18, 2026 at 09:06:02PM +1200, David Rowley wrote: > On Fri, 10 Oct 2025 at 11:30, Michael Paquier <[email protected]> wrote: > > Thanks for double-checking. Applied after running an indent. > > I was working on test_bitmapset.c to add some tests for a new > bitmapset function. I noticed a few weird things. > > 1. test_random_operations() is coded to use GetCurrentTimestamp() as a > seed when the given seed is <= 0. Of course, it'll be a while before > the return value of that wraps beyond 2^63 (292250 years), but I still > can't help but think that NULL is a better value to use to have the > seed auto-generate.
Fine by me. > 2. Doing #1 means the function can't be STRICT. I do think it's wrong > that the function is marked as strict. That's normally reserved for > functions that we needn't call because NULL input(s) yield a NULL > output. That's not the case for this function. Using the existing HEAD approach where STRICT avoids these extra NULL checks, or adding explicit NULL checks without STRICT does not strike me as a big difference in this context. > 3. There's no CHECK_FOR_INTERRUPTS() in test_random_operations(). If > someone uses a large num_ops, there's no way to cancel the query. > 4. If there happened to be some rare bug in bitmapset.c that > test_random_operations() we might struggle to find it again, as we > don't report which seed we used in the ERROR message. These make sense. > I felt it was worth fixing these now as the function I plan to add > there does #1, #2, #3 and #4. If I add the new function for v20, the > discrepancy seems questionable. It is a test module, it would be a big issue if new pieces are backpatched in this area. In short I'm fine with these. Thanks for asking. -- Michael
signature.asc
Description: PGP signature
