On Fri, 15 Aug 2025 at 03:14, Tom Lane <[email protected]> wrote: > > David Rowley <[email protected]> writes: > > It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the > > code does "prevbit--;". Maybe it would be less confusing if it were > > written as: > > * "prevbit" must be less than or equal to "a->nwords * > > BITS_PER_BITMAPWORD". > > The Assert should be using <= rather than <. > > Actually, I don't agree with that. It's true that it wouldn't fail, > but a caller doing that is exhibiting undue intimacy with the innards > of Bitmapsets. The expected usage is that the argument is initially > -1 and after that the result of the previous call (which'll > necessarily be less than a->nwords * BITS_PER_BITMAPWORD). We don't > have any state with which we can verify the chain of calls, but it > seems totally reasonable to me to disallow an outside caller > providing an argument >= a->nwords * BITS_PER_BITMAPWORD.
I can't get on board with this way of thinking. I'm happy enough that we add an Assert, but if we're going to do that then the Assert should be coded in a way that's aligned to the actual restriction that we're trying to protect against. I've heard people arguing before that Asserts can act as documentation of what are valid values for a given function parameter. If that's true, then we should code the Assert so it checks for *valid* values, not 1 less than a valid value. IMO, doing it any other way is likely to cause confusion in people who want to use the function, or bug reports that we're off by 1 in the Assert. David
