On Sun, Jan 21, 2018 at 2:40 AM, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx> wrote: > On Sat, Jan 20, 2018 at 01:06:09PM -0800, Dan Williams wrote: >> +/* >> + * If idx is negative or if idx > size then bit 63 is set in the mask, >> + * and the value of ~(-1L) is zero. When the mask is zero, bounds check >> + * failed, array_ptr will return NULL. > > The more times I see this the more times I'm unhappy with this comment: > > 1. does this really mean "idx > size" or "idx >= size" ? The code > implements the latter not the former. > > 2. is "bit 63" relevant here - what if longs are 32-bit? "the top bit" > or "the sign bit" would be better. > > 3. "and the value of ~(-1L) is zero." So does this mean that when > 0 <= idx < size, somehow the rules of logic change and ~(-1L) > magically becomes no longer zero! > > I'd suggest changing the description to something like: > > * If 0 <= idx < size, return a mask of ~0UL, otherwise return zero. > > or: > > * When idx is out of bounds (iow, is negative or idx >= sz), the sign > * bit will be set. Extend the sign bit to all bits and invert, giving > * a result of zero for an out of bounds idx, or ~0UL if within bounds. > > depending on how deeply you want to describe what's going on here. Sounds good Russell, I'll fold the longer description in for the next version to save the next person needing to decode this technique.
