primitiveDisplayString fails on empty Strings

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

primitiveDisplayString fails on empty Strings

fniephaus
 
Hi all,

I noticed that BitBlt's primitiveDisplayString fails relatively often and then found out that it's sometimes called to draw empty strings.

`(ByteString allInstances select: [:ea | ea isEmpty]) size` revealed that there are 2245 empty strings in my image and I assume some of them must be either in the FrameRateMorph morph or in the Clock morph.

Anyway...looking at BitBlt>>#primDisplayString:from:to:map:xTable:kern:, the fallback code basically does nothing if stopIndex is zero (`startIndex to: stopIndex do: ...`) which makes sense.

What I think doesn't make sense is that the primitive fails in the first place. Instead of failing if stopIndex is not greater 0, I would suggest to use: `stopIndex < 1 ifTrue: [^ interpreterProxy pop: 6].` which cleans up the stack, leaves the receiver, and returns early.

Let me know what you think!


Fabio
Reply | Threaded
Open this post in threaded view
|

Re: primitiveDisplayString fails on empty Strings

Nicolas Cellier
 
+1

2018-05-23 21:11 GMT+02:00 Fabio Niephaus <[hidden email]>:
 
Hi all,

I noticed that BitBlt's primitiveDisplayString fails relatively often and then found out that it's sometimes called to draw empty strings.

`(ByteString allInstances select: [:ea | ea isEmpty]) size` revealed that there are 2245 empty strings in my image and I assume some of them must be either in the FrameRateMorph morph or in the Clock morph.

Anyway...looking at BitBlt>>#primDisplayString:from:to:map:xTable:kern:, the fallback code basically does nothing if stopIndex is zero (`startIndex to: stopIndex do: ...`) which makes sense.

What I think doesn't make sense is that the primitive fails in the first place. Instead of failing if stopIndex is not greater 0, I would suggest to use: `stopIndex < 1 ifTrue: [^ interpreterProxy pop: 6].` which cleans up the stack, leaves the receiver, and returns early.

Let me know what you think!


Fabio


Reply | Threaded
Open this post in threaded view
|

Re: primitiveDisplayString fails on empty Strings

Eliot Miranda-2
In reply to this post by fniephaus
 
Hi Fabio,



On Wed, May 23, 2018 at 12:11 PM, Fabio Niephaus <[hidden email]> wrote:
 
Hi all,

I noticed that BitBlt's primitiveDisplayString fails relatively often and then found out that it's sometimes called to draw empty strings.

`(ByteString allInstances select: [:ea | ea isEmpty]) size` revealed that there are 2245 empty strings in my image and I assume some of them must be either in the FrameRateMorph morph or in the Clock morph.

Anyway...looking at BitBlt>>#primDisplayString:from:to:map:xTable:kern:, the fallback code basically does nothing if stopIndex is zero (`startIndex to: stopIndex do: ...`) which makes sense.

What I think doesn't make sense is that the primitive fails in the first place. Instead of failing if stopIndex is not greater 0, I would suggest to use: `stopIndex < 1 ifTrue: [^ interpreterProxy pop: 6].` which cleans up the stack, leaves the receiver, and returns early.

Let me know what you think!

I don't object.  So you suggest the following?

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex > 0
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

=>

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex >= 0 "to avoid failing for empty strings..."
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].
 
stopIndex = 0 ifTrue:
[^interpreterProxy pop: 6. "the string is empty; pop args, return rcvr"].

Any other opinions either way?

_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: primitiveDisplayString fails on empty Strings

fniephaus
 
Hi Eliot,


On Wed, May 23, 2018 at 9:48 PM Eliot Miranda <[hidden email]> wrote:
 
Hi Fabio,



On Wed, May 23, 2018 at 12:11 PM, Fabio Niephaus <[hidden email]> wrote:
 
Hi all,

I noticed that BitBlt's primitiveDisplayString fails relatively often and then found out that it's sometimes called to draw empty strings.

`(ByteString allInstances select: [:ea | ea isEmpty]) size` revealed that there are 2245 empty strings in my image and I assume some of them must be either in the FrameRateMorph morph or in the Clock morph.

Anyway...looking at BitBlt>>#primDisplayString:from:to:map:xTable:kern:, the fallback code basically does nothing if stopIndex is zero (`startIndex to: stopIndex do: ...`) which makes sense.

What I think doesn't make sense is that the primitive fails in the first place. Instead of failing if stopIndex is not greater 0, I would suggest to use: `stopIndex < 1 ifTrue: [^ interpreterProxy pop: 6].` which cleans up the stack, leaves the receiver, and returns early.

Let me know what you think!

I don't object.  So you suggest the following?

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex > 0
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

=>

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex >= 0 "to avoid failing for empty strings..."
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

stopIndex = 0 ifTrue:
[^interpreterProxy pop: 6. "the string is empty; pop args, return rcvr"]. 

Any other opinions either way?

Why not do the stopIndex = 0 check early and move it all the way up? The primitive won't fail if any of the other checks fail which I don't think is bad if you don't want to draw anything (stopIndex = 0). On the other hand, these other checks are not executed at all (e.g. loadBitBltFrom:) and you don't have to change the stopIndex > 0 check. :)

Fabio
 

_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: primitiveDisplayString fails on empty Strings

Eliot Miranda-2
 
Hi Fabio,

On Wed, May 23, 2018 at 1:22 PM, Fabio Niephaus <[hidden email]> wrote:
 
Hi Eliot,


On Wed, May 23, 2018 at 9:48 PM Eliot Miranda <[hidden email]> wrote:
 
Hi Fabio,



On Wed, May 23, 2018 at 12:11 PM, Fabio Niephaus <[hidden email]> wrote:
 
Hi all,

I noticed that BitBlt's primitiveDisplayString fails relatively often and then found out that it's sometimes called to draw empty strings.

`(ByteString allInstances select: [:ea | ea isEmpty]) size` revealed that there are 2245 empty strings in my image and I assume some of them must be either in the FrameRateMorph morph or in the Clock morph.

Anyway...looking at BitBlt>>#primDisplayString:from:to:map:xTable:kern:, the fallback code basically does nothing if stopIndex is zero (`startIndex to: stopIndex do: ...`) which makes sense.

What I think doesn't make sense is that the primitive fails in the first place. Instead of failing if stopIndex is not greater 0, I would suggest to use: `stopIndex < 1 ifTrue: [^ interpreterProxy pop: 6].` which cleans up the stack, leaves the receiver, and returns early.

Let me know what you think!

I don't object.  So you suggest the following?

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex > 0
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

=>

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex >= 0 "to avoid failing for empty strings..."
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

stopIndex = 0 ifTrue:
[^interpreterProxy pop: 6. "the string is empty; pop args, return rcvr"]. 

Any other opinions either way?

Why not do the stopIndex = 0 check early and move it all the way up? The primitive won't fail if any of the other checks fail which I don't think is bad if you don't want to draw anything (stopIndex = 0). On the other hand, these other checks are not executed at all (e.g. loadBitBltFrom:) and you don't have to change the stopIndex > 0 check. :)

Because it feels hack to me to not fail the primitive if one supplied an empty string but an otherwise corrupted bitblt obj.  IMO, the primitive should succeed only if all its input arguments are correct.  Pushing the stopIndex check earlier circumvents these checks.  I'll commit the above and if you feel strongly you can commit an updated version (that documents your preference).
 
Fabio
 

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


_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: primitiveDisplayString fails on empty Strings

fniephaus
 
Hi Eliot,

On Thu, May 24, 2018 at 9:51 PM Eliot Miranda <[hidden email]> wrote:
 
Hi Fabio,

On Wed, May 23, 2018 at 1:22 PM, Fabio Niephaus <[hidden email]> wrote:
 
Hi Eliot,


On Wed, May 23, 2018 at 9:48 PM Eliot Miranda <[hidden email]> wrote:
 
Hi Fabio,



On Wed, May 23, 2018 at 12:11 PM, Fabio Niephaus <[hidden email]> wrote:
 
Hi all,

I noticed that BitBlt's primitiveDisplayString fails relatively often and then found out that it's sometimes called to draw empty strings.

`(ByteString allInstances select: [:ea | ea isEmpty]) size` revealed that there are 2245 empty strings in my image and I assume some of them must be either in the FrameRateMorph morph or in the Clock morph.

Anyway...looking at BitBlt>>#primDisplayString:from:to:map:xTable:kern:, the fallback code basically does nothing if stopIndex is zero (`startIndex to: stopIndex do: ...`) which makes sense.

What I think doesn't make sense is that the primitive fails in the first place. Instead of failing if stopIndex is not greater 0, I would suggest to use: `stopIndex < 1 ifTrue: [^ interpreterProxy pop: 6].` which cleans up the stack, leaves the receiver, and returns early.

Let me know what you think!

I don't object.  So you suggest the following?

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex > 0
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

=>

((interpreterProxy isArray: xTable)
and: [(interpreterProxy isArray: glyphMap)
and: [(interpreterProxy slotSizeOf: glyphMap) = 256
and: [(interpreterProxy isBytes: sourceString)
and: [startIndex > 0
and: [stopIndex >= 0 "to avoid failing for empty strings..."
and: [stopIndex <= (interpreterProxy byteSizeOf: sourceString)
and: [(self loadBitBltFrom: bbObj)
and: [combinationRule ~= 30 "these two need extra source alpha"
and: [combinationRule ~= 31]]]]]]]]]) ifFalse:
[^interpreterProxy primitiveFail].

stopIndex = 0 ifTrue:
[^interpreterProxy pop: 6. "the string is empty; pop args, return rcvr"]. 

Any other opinions either way?

Why not do the stopIndex = 0 check early and move it all the way up? The primitive won't fail if any of the other checks fail which I don't think is bad if you don't want to draw anything (stopIndex = 0). On the other hand, these other checks are not executed at all (e.g. loadBitBltFrom:) and you don't have to change the stopIndex > 0 check. :)

Because it feels hack to me to not fail the primitive if one supplied an empty string but an otherwise corrupted bitblt obj.  IMO, the primitive should succeed only if all its input arguments are correct.  Pushing the stopIndex check earlier circumvents these checks.  I'll commit the above and if you feel strongly you can commit an updated version (that documents your preference).

All good and thanks! This is the first time I'm proposing changes to the VMMaker code base, so I'm just learning best practices from you here :)

Fabio
 

Fabio


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


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