On Tue, May 13, 2025 at 11:48:36AM +0200, Patrick Steinhardt wrote: > On Mon, May 12, 2025 at 02:51:30PM -0400, Jeff King wrote: > > Callers which want to know how many items are in an oidmap have to look > > at the underlying hashmap struct, leaking an implementation detail. > > Let's provide a type-appropriate wrapper and use it. > > > > Note in the call from lookup_replace_object(), the caller was actually > > looking at the hashmap's tablesize parameter (the allocated size of the > > table) rather than hashmap_get_size(), the number of items in the table. > > This probably should have been checking the number of items all along, > > but the two are functionally equivalent here since we only add to the > > map and never remove anything. Thus if there was any allocation, it was > > because there is at least one item. > > I was a bit puzzled by this explanation initially. The two sizes aren't > functioally equivalent -- the table size will typically be larger than > the number of contained entries. But the thing is that we don't care for > the actual size, we only care whether the map is empty or not. And for > that those are indeed equivalent in this specific case. Yep, exactly. Probably replacing "looking at X" with "checking whether X is empty" would have been more clear. I of course was looking at the diff while writing this, so took it for granted. ;) I don't think it's worth a re-roll, though, especially since this is in next already. Thanks for the review. -Peff
