#[195 164] collect: [:c | c hex] => #[16 16]
I think this should have raised an error. Tim?
- Bert -
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?
_,,,^..^,,,_ best, Eliot
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 Wed, Jul 19, 2017 at 11:16 PM, 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.
Spoke to Tim. 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
Also, ByteArray may not be the only class suffering from this problem: SystemNavigation default browseAllSelect: [:cm | cm primitive = 105]
- Bert -
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 -
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
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
Technically 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 -
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
Technically 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 -
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 works in 5.1
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
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
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
Technically 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.
On 4/26/18, Eliot Miranda eliot.miranda@gmail.com wrote:
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
Technically 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.
+1 (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.
squeak-dev@lists.squeakfoundation.org