Hi Eliot,

If we want to keep the methods in the image, then I see two possibilities:
- One of them is, as you suggested, to add the methods to ExternalAddress. 
This is a lot cleaner than relying on inheritance, and would also avoid 
the updating issue you mentioned. It's fully backwards-compatible as well 
- with these changes FFI can still be used from older images.
- Another option would be to move the primitive from the FFI plugin to the 
VM. I didn't check how the primitive works, but I assume that it uses the 
external objects array to get known of ExternalAddress, so I think this 
is also possible. This way we could use the primitive from the image 
side code as well, which should be faster especially for larger numbers.

Btw, there are other semantic differences between the two implementations. 
The primitive is always strict about the accepted range of input values, 
but the image side code is more lenient about under- and overflows, which 
allows some tricks. E.g.:

 	(ByteArray new: 1) signedByteAt: 1 put: 255; signedByteAt: 1

returns -1 using the code in Trunk, but it will fail with the primitive.

Levente

On Tue, 16 Feb 2016, Eliot Miranda wrote:

> Hi Levente,
> 
>     I just wanted to note a potential problem with Collections-ul.667
> 
> Name: Collections-ul.667
> Author: ul
> Time: 10 October 2015, 3:40:46.732 pm
> UUID: 7cd785ff-1839-4075-ac56-4b71054529d8
> Ancestors: Collections-topa.666
> 
> Added the missing methods of ByteArray's platform independent access category: #signedByteAt: #signedByteAt:put: #long64At:put:bigEndian: and #long64At:bigEndian:. The 64-bit methods are not
> optimized.
> 
> 
> In this version you provided ByteArray>>#signedByteAt:[put:] which conflicts with FFI-Kernel's methods of the same name.  There's a difference in semantics.  If you look at
> ExternalStructure's class comment it points out that
> 
> "This class provides an abstract base for all structures that can be used by external functions. ExternalStructures have two possible handle types:
> - ExternalAddress
> If the handle is an external address then the object described does not reside in the Smalltalk object memory.
> - ByteArray
> If the handle is a byte array then the object described resides in Smalltalk memory.
> Useful methods should be implemented by subclasses of ExternalStructure using the common ByteArray/ExternalAddress platform dependent access protocol which will transparently access the
> correct memory location."
> 
> i.e. the primitives used automatically differentiate between an ExternalAddress holding a pointer to external memory, in which case the datum is accessed via that location, or a ByteArray,
> in which case the datum is accessed in the ByteArray.  Your versions only do the latter and would hence misinterpret data in ExternalAdderss instances.  Here's the FFI code:
> 
> ByteArray>>signedByteAt: byteOffset
> "Return a 8bit signed integer starting at the given byte offset"
> ^self integerAt: byteOffset size: 1 signed: true
> 
> ByteArray>>integerAt: byteOffset size: nBytes signed: aBoolean
> "Primitive. Return an integer of nBytes size from the receiver.
> Note: This primitive will access memory in the outer space if
> invoked from ExternalAddress."
> <primitive: 'primitiveFFIIntegerAt' module:'SqueakFFIPrims'>
> ^self primitiveFailed
> 
> And here's yours:
> ByteArray>>signedByteAt: index
> "Return an 8-bit signed integer quantity from the given byte index."
> | byte |
> (byte := self at: index) <= 16r7F ifTrue: [ ^byte ].
> ^byte - 16r100
> 
> Now of course this isn't an issue in builds that load the FFI after Collections, but it does affect images in which FFI-Kernel was loaded and then the image was updated to
> instal Collections-ul.667.
> 
> I'm not sure what the right fix is, but I thought I'd better mention it.  One obvious one is to implement the FFI methods in ExternalAddress>>#signedByteAt:[put:], and if folks agree I can
> do this.  Let me know.  Pharoers, if I do this, you'll need to make sure that the Pharo base includes ByteArray>>#signedByteAt:[put:].
> 
> _,,,^..^,,,_
> best, Eliot
> 
>