Re: Strange Bitmapset manipulation in DiscreteKnapsack()

On Tue, Jan 16, 2024 at 11:32 AM David Rowley <dgrowleyml@gmail.com> wrote:

> While working on [1], I noticed some strange code in
> DiscreteKnapsack() which seems to be aiming to copy the Bitmapset.
>
> It's not that obvious at a casual glance, but:
>
> sets[j] = bms_del_members(sets[j], sets[j]);
>
> this is aiming to zero all the words in the set by passing the same
> set in both parameters.
>
> Now that 00b41463c changed Bitmapset to have NULL be the only valid
> representation of an empty set, this code no longer makes sense.  We
> may as well just bms_free() the original set and bms_copy() in the new
> set as the bms_del_members() call will always pfree the set anyway.
>
> I've done that in the attached.


+1.  This is actually what happens with the original code, i.e.
bms_del_members() will pfree sets[j] and bms_add_members() will bms_copy
sets[ow] to sets[j].  But the new code looks more natural.

I also checked other callers of bms_del_members() and did not find
another case that passes the same set in both parameters.

Thanks
Richard