PositionableStream>>upTo: anObject

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

PositionableStream>>upTo: anObject

stepharo
While browsing the system I saw that the following method raised a temp
read before written...

It did not jump to my eyes. Working on something else....


PositionableStream>>upTo: anObject
     "Answer a subcollection from the current access position to the
     occurrence (if any, but not inclusive) of anObject in the receiver. If
     anObject is not in the collection, answer the entire rest of the
receiver."
     | newStream element |
     newStream := (collection species new: 100) writeStream.
     [self atEnd or: [(element := self next) = anObject]]
         whileFalse: [newStream nextPut: element].
     ^newStream contents


Reply | Threaded
Open this post in threaded view
|

Re: PositionableStream>>upTo: anObject

Nicolas Cellier
initialization of element is conditional (in the or: block).
I'm amazed, that's a new behavior, is it a consequence of using AST?
But a dumb compiler not knowing the semantic of or: (it is a message like others) could conclude that element may be used uninitialized in the whileFalse block...
A compiler that is inlining or: with a well known semantic has no excuse ;)

2016-08-16 22:12 GMT+02:00 stepharo <[hidden email]>:
While browsing the system I saw that the following method raised a temp read before written...

It did not jump to my eyes. Working on something else....


PositionableStream>>upTo: anObject
    "Answer a subcollection from the current access position to the
    occurrence (if any, but not inclusive) of anObject in the receiver. If
    anObject is not in the collection, answer the entire rest of the receiver."
    | newStream element |
    newStream := (collection species new: 100) writeStream.
    [self atEnd or: [(element := self next) = anObject]]
        whileFalse: [newStream nextPut: element].
    ^newStream contents



Reply | Threaded
Open this post in threaded view
|

Re: PositionableStream>>upTo: anObject

John Brant-2
I’m guessing that it is using the read before written tester from the refactoring browser. It treats ifTrue:ifFalse:/ifFalse:ifTrue: blocks and the receiver blocks of whileTrue/False: specially. All other blocks are considered potentially not executed. Therefore, it says that the or: block may not be executed so the “element” reference in the whileFalse: argument block may be read without any assignment.

The RB’s read before written tester is different from the compilers. The compiler’s version lets some things through that it shouldn’t, and I think the compiler should use the RB version instead. Here’s an example where the compiler generates incorrect code for the optimized “v” variable in the whileTrue:

| stream count |
count := 0.
stream := ReadStream on: #(true true true false false).
[ stream next and: [ | v | stream next ifTrue: [ v := 1 ]. v notNil ] ]
        whileTrue: [ count := count + 1 ].
count = 1

This should answer true, but answers false. The compiler should see that “v” may potentially be read before written, and therefore assign it to nil at the beginning of the or: block like would be done for an unoptimized block temporary. If you perform the #whileTrue: instead (to get a real block), you get the correct result:

| stream count |
count := 0.
stream := ReadStream on: #(true true true false false).
[ stream next and: [ | v | stream next ifTrue: [ v := 1 ]. v notNil ] ]
        perform: #whileTrue: with: [ count := count + 1 ].
count = 1


John Brant


> On Aug 16, 2016, at 3:23 PM, Nicolas Cellier <[hidden email]> wrote:
>
> initialization of element is conditional (in the or: block).
> I'm amazed, that's a new behavior, is it a consequence of using AST?
> But a dumb compiler not knowing the semantic of or: (it is a message like others) could conclude that element may be used uninitialized in the whileFalse block...
> A compiler that is inlining or: with a well known semantic has no excuse ;)
>
> 2016-08-16 22:12 GMT+02:00 stepharo <[hidden email]>:
> While browsing the system I saw that the following method raised a temp read before written...
>
> It did not jump to my eyes. Working on something else....
>
>
> PositionableStream>>upTo: anObject
>     "Answer a subcollection from the current access position to the
>     occurrence (if any, but not inclusive) of anObject in the receiver. If
>     anObject is not in the collection, answer the entire rest of the receiver."
>     | newStream element |
>     newStream := (collection species new: 100) writeStream.
>     [self atEnd or: [(element := self next) = anObject]]
>         whileFalse: [newStream nextPut: element].
>     ^newStream contents
>
>
>


Reply | Threaded
Open this post in threaded view
|

Re: PositionableStream>>upTo: anObject

Nicolas Cellier


2016-08-16 23:19 GMT+02:00 John Brant <[hidden email]>:
I’m guessing that it is using the read before written tester from the refactoring browser. It treats ifTrue:ifFalse:/ifFalse:ifTrue: blocks and the receiver blocks of whileTrue/False: specially. All other blocks are considered potentially not executed. Therefore, it says that the or: block may not be executed so the “element” reference in the whileFalse: argument block may be read without any assignment.


Since or: and: are pretty much like ifTrue: ifFalse: they should better be treated as specially.
 
The RB’s read before written tester is different from the compilers. The compiler’s version lets some things through that it shouldn’t, and I think the compiler should use the RB version instead. Here’s an example where the compiler generates incorrect code for the optimized “v” variable in the whileTrue:

| stream count |
count := 0.
stream := ReadStream on: #(true true true false false).
[ stream next and: [ | v | stream next ifTrue: [ v := 1 ]. v notNil ] ]
        whileTrue: [ count := count + 1 ].
count = 1

This should answer true, but answers false. The compiler should see that “v” may potentially be read before written, and therefore assign it to nil at the beginning of the or: block like would be done for an unoptimized block temporary. If you perform the #whileTrue: instead (to get a real block), you get the correct result:

| stream count |
count := 0.
stream := ReadStream on: #(true true true false false).
[ stream next and: [ | v | stream next ifTrue: [ v := 1 ]. v notNil ] ]
        perform: #whileTrue: with: [ count := count + 1 ].
count = 1


John Brant


Yes, you're right, that's a Compiler bug.
The result shall not change whether blocks are optimized or not.



> On Aug 16, 2016, at 3:23 PM, Nicolas Cellier <[hidden email]> wrote:
>
> initialization of element is conditional (in the or: block).
> I'm amazed, that's a new behavior, is it a consequence of using AST?
> But a dumb compiler not knowing the semantic of or: (it is a message like others) could conclude that element may be used uninitialized in the whileFalse block...
> A compiler that is inlining or: with a well known semantic has no excuse ;)
>
> 2016-08-16 22:12 GMT+02:00 stepharo <[hidden email]>:
> While browsing the system I saw that the following method raised a temp read before written...
>
> It did not jump to my eyes. Working on something else....
>
>
> PositionableStream>>upTo: anObject
>     "Answer a subcollection from the current access position to the
>     occurrence (if any, but not inclusive) of anObject in the receiver. If
>     anObject is not in the collection, answer the entire rest of the receiver."
>     | newStream element |
>     newStream := (collection species new: 100) writeStream.
>     [self atEnd or: [(element := self next) = anObject]]
>         whileFalse: [newStream nextPut: element].
>     ^newStream contents
>
>
>



Reply | Threaded
Open this post in threaded view
|

Re: PositionableStream>>upTo: anObject

John Brant-2

> On Aug 16, 2016, at 5:45 PM, Nicolas Cellier <[hidden email]> wrote:
>
> 2016-08-16 23:19 GMT+02:00 John Brant <[hidden email]>:
> I’m guessing that it is using the read before written tester from the refactoring browser. It treats ifTrue:ifFalse:/ifFalse:ifTrue: blocks and the receiver blocks of whileTrue/False: specially. All other blocks are considered potentially not executed. Therefore, it says that the or: block may not be executed so the “element” reference in the whileFalse: argument block may be read without any assignment.
>
>
> Since or: and: are pretty much like ifTrue: ifFalse: they should better be treated as specially.

#ifTrue: and #ifFalse: blocks aren’t handled specially since we can’t guarantee that the block is evaluated. #ifTrue:ifFalse: and #ifFalse:ifTrue: are handled specially since we can guarantee that one of the two blocks are executed. Therefore, if the variable is assigned before read in both blocks we can say that it is assigned before read for the rest of the method. I never implemented and:/or: support in the read before written tester since for the refactorings that I performed it was never a problem, and it is more complicated to implement. For example, you have to look at and:/or:’s in combination with outer and:/or:/ifTrue:{ifFalse:}/ifFalse:{ifTrue:}/whileTrue{:}/whileFalse{:} messages. Furthermore, you need to look to see if the opposite side of the condition contains a return. For example, we could have something like this:

        (condition and: [variable := otherCondition]) ifFalse: [^self].
        self doSomethingWith: variable

Here “variable” is always assigned before read since “condition” had to be true to get to the #doSomethingWith: line.


John Brant