On Tue, Jan 16, 2024 at 11:32 AM David Rowley <dgrowle...@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
