On Fri, Feb 03, 2023 at 11:44:30PM +0000, Eric Wong wrote: > > /* If we aren't using islands, assume everything goes together. */ > > - if (!island_marks) > > + if (!using_island_marks) > > return 1; > > I much prefer to rely on invalid pointers than extra flags since > having multiple sources of truth confuses me[1]. That's kind of my point, though. It's not multiple sources of truth, but rather there are two bits "did we ask to use islands" and "is island_marks still valid". You are shoving both bits into the same variable by using a special sentinel pointer. > > And of course that would also be a tiny step in the right direction if > > the delta islands API learned to use a struct (this would be the same > > spot where we'd say "we're done with islands; free the struct"). > > I do wonder about performance on register-starved systems, > though, especially if stuff like island_delta_cmp gets called > frequently. I already have enough performance problems atm :< Calling in_same_island() is pretty heavy-weight (it's multiple hash lookups, and then an arbitrary-length bit-string comparison). I'd be shocked if replacing a global with a struct pointer is even measurable. > [1] to go farther, I might even eliminate `int use_delta_islands' as > a global from builtin/pack-objects.c and just have that become a > `struct delta_islands_foo *' or something. But I have more > pressing performance problems to figure out :< Right, that's along the same lines I was thinking. But I don't blame you for not tackling it. The upside is fairly minimal. > + /* detect use-after-free with a an address which is never valid: */ > + island_marks = (void *)-1; I still hate how magical this line is, but I don't that it's worth arguing about more. -Peff
