On Tue, Jul 1, 2014 at 12:21 PM, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> wrote:

 

2014-07-01 4:22 GMT+02:00 <commits@source.squeak.org>:

Item was changed:
  ----- Method: LargeIntegersPlugin>>cDigitSub:len:with:len:into: (in category 'C core') -----
+ cDigitSub: pByteSmall len: smallLen with: pByteLarge len: largeLen into: pByteRes
+       | z |
- cDigitSub: pByteSmall
-               len: smallLen
-               with: pByteLarge
-               len: largeLen
-               into: pByteRes
-       | z limit |
        <var: #pByteSmall type: 'unsigned char * '>
        <var: #pByteLarge type: 'unsigned char * '>
        <var: #pByteRes type: 'unsigned char * '>

+       z := 0. "Loop invariant is -1<=z<=1"
+       0 to: smallLen - 1 do:
-       z := 0.
-       "Loop invariant is -1<=z<=1"
-       limit := smallLen - 1.
-       0 to: limit do:
                [:i |
                z := z + (pByteLarge at: i) - (pByteSmall at: i).
+               pByteRes at: i put: z - (z // 256 * 256). "sign-tolerant form of (z bitAnd: 255)"

Frankly, having z declared unsigned int and just doing  pByteRes at: i put: (z bitAnd: 16rFF) as I suggested would be way way simpler and will ALWAYS work.

Why the hell invoke the complications of signed arithmetic when the content pByteRes is unsigned???

I'm not maintaining the plugin.  But I broke it in fixing the unsigned division anomaly.  I just wanted it to work again as quickly as possibly without expending effort.  I made the minimum changes I could to keep it working.  I'm much happier to have you maintain the plugin.  You have the expertise and experience.

Nicolas, my priority is to have Spur working.  I don't want to have to expend lots of energy changing plugins to get Spur working.  My submitting this fix is not an endorsement of any kind.  It's merely expediency.

Even if z is declared signed, (z bitAnd: 255) would work on a vast majority of compiler/processor because most compiler/processor would use a 2-complement representation.

But it's technically implementation-defined.

+               z := z signedBitShift: -8].

i think this one is OK too on a vast majority of machines, but this is as well technically implementation defined.

Some weird compiler/processor pair may as well fill left bits with zeroes, or not even use 2-complement...

z here is a carry and should contain either 0, or -1 (that is UINT_MAX) after this operation.

 

+       smallLen to: largeLen - 1 do:
-               pByteRes at: i put: z - (z // 256 * 256).
-               "sign-tolerant form of (z bitAnd: 255)"
-               z := z // 256].
-       limit := largeLen - 1.
-       smallLen to: limit do:
                [:i |
                z := z + (pByteLarge at: i) .
+               pByteRes at: i put: z - (z // 256 * 256). "sign-tolerant form of (z bitAnd: 255)"
+               z := z signedBitShift: -8].
-               pByteRes at: i put: z - (z // 256 * 256).
-               "sign-tolerant form of (z bitAnd: 255)"
-               z := z // 256].
  !

Isn't there another funny signed shift in Large Integer division?

This would deserve a review too, because division is inlining its own sort of subtraction...

--
best,

Eliot