Re: [PATCH] Add tests for Bitmapset

On Oct 8 2025, at 11:47 pm, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Thu, Oct 09, 2025 at 04:35:55PM +1300, David Rowley wrote:
>> On Thu, 9 Oct 2025 at 15:13, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> > What do you think about the attached?
>>
>> Thanks. Looks pretty good.
>>
>> > + members = palloc(sizeof(int) * num_ops);
>>
>> Any reason to pfree that and allocate that to the same size as it
>> already was?
>
> No reason. We can shortcut that a bit.
>
>> Wondering if the "members[pos] = members[--num_members];" is worth a
>> short comment. Maybe something like: /* zap this member by moving the
>> final array member into its place and shrinking the array by 1 */
>
> Yes, a comment can be adapted here. Sounds good to me.
> --
> Michael

Michael, David,

This patch LGTM. I appreciate the additional work you've both put into it.

You're right, of course, I didn’t need to preserve the order of elements
when deleting. This is where I'd started and I don't recall why I
changed it and didn't revert to this simpler solution.

+ members[pos] = members[--num_members];

Phase 2 might record a value to members that was also recorded in phase
1, but that seems harmless. That's why I had:

+ if (!bms_is_member(member, bms1) && !bms_is_member(member, bms2))

I hadn't thought to optimize this function, but given the amount of time
the tests run in aggregate globally, it makes sense.

best.

-greg