Re: [libvirt] [PATCH 2/4] fixup? util: Optimize virBitmapUnion()

On Mon, 2019-06-03 at 16:43 +0200, Ján Tomko wrote: > On Mon, Jun 03, 2019 at 04:31:51PM +0200, Andrea Bolognani wrote: > > On Mon, 2019-06-03 at 14:10 +0200, Ján Tomko wrote: > > > if (b->nbits && > > > a->nbits < b->nbits && > > > virBitmapExpand(a, b->nbits -1) < 0) { > > > > Yeah, you're right, we need to account for the zero-indexing of bits. > > I mean, it's not like the resulting bitmap would be incorrect either > > way, but we might end up allocating more memory than it's actually > > required. > > Actually I do think it would be incorrect. The bitmap size is as > important as its values - e.g. if the resulting bitmap had 4 set bits, > a size of 4 vs 5 would alter the result of virBitmapIsAllSet. Oh, right, that's something that I hadn't considered. This is pretty much exactly why I didn't trust my implementation to be correct O:-) > > The first check seems unnecessary, though: the only case in which the > > argument to virBitmapExpand() would be incorrect is b->nbits == 0, > > but we know that both a->nbits and b->nbits are >= 0 and we also just > > verified that a->nbits < b->nbits, so b->nbits must be >= 1 and the > > argument to virBitmapExpand() will always be correct. Or am I missing > > something? > > Right, it will work unless someone changes nbits from size_t to double. So ACK to drop the first check while adopting your version of that hunk?