On 5/13/19 5:14 PM, Andrew Dinn wrote: > Thank you for looking at the patch. > > On 28/04/2019 18:09, Andrew Haley wrote: >> On 4/25/19 5:34 PM, Andrew Dinn wrote: >>> long map_base = (address & ~(ps - 1)); >> >> This looks like a hard way to write >> >> long map_base = address & -ps; > > My version certainly uses more characters, that is for sure. However, I > chose (and still prefer) this form for /clarity/. It seems to me to > communicate as simply as possible what mask is being constructed, > granted the starting premise that ps is a power of 2 (i.e. has only a > single bit set). > > It only requires elementary knowledge of binary representations to see: > > firstly, that ps-1 clears the original bit and sets all lower bits;
I think your core argument fails at this point. You have *already* moved to considering a bitwise transformation as arithmetic. > next that ~(ps-1) clears those lower bits and sets all bits from the > original (inclusive) upwards; > > then that the result can be used as a mask to clear those same bottom > bits from address; > > finally that this mask operation is equivalent to rounding down to > the relevant power of two. > > I find your alternative (a tad) less clear because it employs an > arithmetic operation to achieve the requisite bitwise transformation. So does yours. See above. > That the two expressions compute to equivalent results requires some > experience and understanding of the twos-complement representations of > numbers rather than basic knowledge of bit fields. > > As a gcc hacker 'your mileage may vary' ;-) > > Crucially, every compiler we rely on is going to produce the same code > in both cases. It helps a lot to think of the operation as sign extending the most significant bit. This clears up the confusion and removes the need for the obfuscation, I think. Still, it's not worth arguing about. You've at least thought about it. -- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
