On 20 July 2017 at 00:16, Eliot Miranda eliot.miranda@gmail.com wrote:

Hi Bert,

On Wed, Jul 19, 2017 at 6:10 AM, Bert Freudenberg bert@freudenbergs.de wrote:

...

#[195 164] collect: [:c | c hex] => #[16 16]

+1

at: index put: value <primitive: 61> "try primitiveAtPut, convert value to integer if that fails and try again" ^ self byteAt: index put: value asInteger

is wrong.

...

I think this should have raised an error. Tim?

Why don't we simply remove the method? Tim may be using this in his benchmarks but I'm sure there's a better way.

But there's possibly another error here too. Why is '16rC3' asInteger 16 instead of 195?

indeed. And that has nothing to do with bytearray method.

_,,,^..^,,,_ best, Eliot

On Thu, Jul 20, 2017 at 12:46 PM, Bert Freudenberg bert@freudenbergs.de wrote:

The problem is actually primitive 105, which allows this:

(ByteArray new: 10) replaceFrom: 1 to: 5 with: 'Hello' => #[72 101 108 108 111 0 0 0 0 0]

If you comment out the primitive call in ByteArray>>replaceFrom:to:with:startingAt: and also remove Tim's at:put:, it will fail. So if a VM does not implement the (optional) primitive 105, the fallback code is wrong.

Using #asInteger in ByteArray>>at:put: is a simple fix for that. Or we have to fix the fallback code in ByteArray>>replaceFrom:to: with:startingAt:.

I just committed the latter to the inbox but I'm not sure I like the implementation: http://source.squeak.org/inbox/Collections-bf.761.diff

I take no response as silent ​agreement, so I moved this to trunk.

Also, ByteArray may not be the only class suffering from this problem: SystemNavigation default browseAllSelect: [:cm | cm primitive = 105]

​Do we think these other places might need fixes?​ ​ - Bert -

On 26 April 2018 at 11:28, marcel.taeumel Marcel.Taeumel@hpi.de wrote:

Hi, there.

I cannot put a ByteString into a ByteArray via a WriteStream anymore:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello'.

You need to do:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello' asByteArray.

This breaks code.

Best, Marcel

​Tech​nically this is an expected error - you simply cannot put Characters (the elements of a String) into a ByteArray. However, for convenience we do support copying Strings into ByteArrays in various places, including WriteStream>>nextPutAll:.

However, the test in WriteStream>>nextPutAll: is too strict. It needs to be "collection class isBits" instead of "collection isString". This makes the code actually match its comment:

WriteStream>>nextPutAll: aCollection

| newEnd | (collection class == aCollection class or: [ collection class isBits and: [ aCollection isString and: [ collection class format = aCollection class format ] ] ]) "Let Strings with the same field size as collection take the quick route too." ifFalse: [ ^ super nextPutAll: aCollection ].

newEnd := position + aCollection size. newEnd > writeLimit ifTrue: [self growTo: newEnd + 10].

collection replaceFrom: position+1 to: newEnd with: aCollection startingAt: 1. position := newEnd. ^aCollection​

With that change your example works. I think this was the original intent of the code.

Fixed in Collections-bf.787

- Bert -

Hi Bert, Hi Marcel,

On Apr 26, 2018, at 5:03 AM, Bert Freudenberg bert@freudenbergs.de wrote:

...

On 26 April 2018 at 13:29, Bert Freudenberg bert@freudenbergs.de wrote:

...

...

On 26 April 2018 at 11:28, marcel.taeumel Marcel.Taeumel@hpi.de wrote:

...

Hi, there.

I cannot put a ByteString into a ByteArray via a WriteStream anymore:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello'.

You need to do:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello' asByteArray.

This breaks code.

Best, Marcel

​Tech​nically this is an expected error - you simply cannot put Characters (the elements of a String) into a ByteArray. However, for convenience we do support copying Strings into ByteArrays in various places, including WriteStream>>nextPutAll:.

However, the test in WriteStream>>nextPutAll: is too strict. It needs to be "collection class isBits" instead of "collection isString". This makes the code actually match its comment:

WriteStream>>nextPutAll: aCollection

| newEnd | (collection class == aCollection class or: [ collection class isBits and: [ aCollection isString and: [ collection class format = aCollection class format ] ] ]) "Let Strings with the same field size as collection take the quick route too." ifFalse: [ ^ super nextPutAll: aCollection ].

newEnd := position + aCollection size. newEnd > writeLimit ifTrue: [self growTo: newEnd + 10].

collection replaceFrom: position+1 to: newEnd with: aCollection startingAt: 1. position := newEnd. ^aCollection​

With that change your example works. I think this was the original intent of the code.

Fixed in Collections-bf.787

​PS: ​doing it this way ensures that you still get an error when you try to write a WideString into a ByteArray stream.

• Bert -

I agree with Sven:

From: Sven Van Caekenberghe sven@stfx.eu Date: April 26, 2018 at 6:36:42 AM PDT To: Pharo Development List pharo-dev@lists.pharo.org Subject: Re: [Pharo-dev] Fwd: ByteArray>>at:put: Reply-To: Pharo Development List pharo-dev@lists.pharo.org

...

On 26 Apr 2018, at 15:21, Sean P. DeNigris sean@clipperadams.com wrote:

Relevant to Pharo?

From http://forum.world.st/ByteArray-at-put-tp4955848.html :

We don't (want to) mix binary and character collections or streams. Going from one to the other is called encoding and decoding, it has to be done while being conscious of which encoding you are using. Sending #asByteArray to a String or #asString to a ByteArray is dangerous, lazy and wrong (in most cases), especially in an international context.

So I would rather see an error or a convenience method such as nextPutAllBits: which does what the above does (implicitly convert) but is explicit. (ByteArray new: 5) writeStream nextPutAll: #[1 2 3 4 5] and (ByteString new: 5) nextPutAll: 'hello' should both raise an error (only the last does), as should (ByteArray new: 5) writeStream nextPut: $!. (ByteString new: 5) nextPut: 33 does. At least these operations should be consistent and symmetrical. Right now nextPut: is consistent with nextPutAll: but String<-Integer is not symmetric with ByteArray<-Character.

When was the last time this worked?

Levente

On Thu, 26 Apr 2018, marcel.taeumel wrote:

Hi, there.

I cannot put a ByteString into a ByteArray via a WriteStream anymore:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello'.

You need to do:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello' asByteArray.

This breaks code.

Best, Marcel

-- Sent from: http://forum.world.st/Squeak-Dev-f45488.html

it got some notice last year:

On 7/29/17 3:15 PM, tim Rowledge wrote:

We recently changed ByteArray>at:put: to remove the backup conversion of the value to integer, for what seemed like decent reasons.

It’s broken my WeatherStation code a little because there are places where I use {my byte stream} nextPutAll: (aString squeakToUtf8) or similar. #squeakToUtf8 returns a bytestring, and of course when the #nextPutAll: loop does its thing each character is pulled out as a Character (even though we know at this point it’s a byte value) - and we’ve just made it impossible to stick a character into a byte array.

Clearly I could fix it reasonably trivially with a few #asByteArray type messages scattered around but it feels a bit tacky somehow. I see some faintly similar code with plausibly similar issues in WebSocket classes too, which would need some care. Not that I can see a lot of usage of that code…

Performance isn’t a colossal issue for MQTT packets but it just rankles a bit to have a known byte valued string and then have to convert it to write it into a byte valued stream collection. KnowWhadIMean?

tim

tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Never write software that anthropomorphizes the machine. They hate that.

On 4/26/18 8:41 AM, Levente Uzonyi wrote:

When was the last time this worked?

Levente

On Thu, 26 Apr 2018, marcel.taeumel wrote:

...

Hi, there.

I cannot put a ByteString into a ByteArray via a WriteStream anymore:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello'.

You need to do:

| s | s := WriteStream on: ByteArray new. s nextPutAll: 'hello' asByteArray.

This breaks code.

Best, Marcel

-- Sent from: http://forum.world.st/Squeak-Dev-f45488.html