On Fri, Jan 14, 2011 at 5:42 AM, Balázs Kósi <rebmekop at gmail.com> wrote:

> Hi,
>
> We fiddled with this a bit.
>
> On Thu, Jan 13, 2011 at 6:52 PM, Eliot Miranda <eliot.miranda at gmail.com>wrote:
>>>
>>> On Thu, Jan 13, 2011 at 9:33 AM, Eliot Miranda <eliot.miranda at gmail.com>wrote:
>>>>
>>>> On Thu, Jan 13, 2011 at 2:29 AM, Bert Freudenberg <bert at freudenbergs.de
>>>> > wrote:
>>>>
>>>>> Here's another decompiler problem that I actually ran into in
>>>>> production code. When decompiling this method, either the block-local temp
>>>>> should remain block-local, or one of the vars needs to be renamed:
>>>>>
>>>>> blockLocalsAndArgs
>>>>>        | product |
>>>>>        product := 1.
>>>>>        1 to: 2 do: [:i |
>>>>>                i = 1
>>>>>                        ifTrue: [ | factor | factor := 6.
>>>>>                                product := product * factor ]
>>>>>                        ifFalse: [ #(9) do: [:factor |
>>>>>                                product := product * factor ] ] ].
>>>>>        ^ product = 13r42
>>>>>
>>>>> This works fine if decompiled without temps but the standard in Squeak
>>>>> is to use the actual temp names - just accept this as a method and switch to
>>>>> decompiled view.
>>>>>
>>>>> The best fix might be if the decompiler always declared the variables
>>>>> in the inner-most block possible. Maybe moving the declarations is not even
>>>>> that hard to do as a post-process on the AST
>>>>
>>>>
>  The Decompiler modified by the attached changeset tries to do this. It
> gives the following output for your example:
>
>   blockLocalsAndArgs
>  | product |
> product := 1.
> 1
> to: 2
>  do: [:i | i = 1
> ifTrue: [| factor |
> factor := 6.
>  product := product * factor]
> ifFalse: [#(9 )
> do: [:factor | product := product * factor]]].
>  ^ product = 54
>
> But it brakes half of the DecompilerTests. Most of them because the
> Decompiler inserts nil assignments to the beginnings of some blocks. eg. [ |
> val | ... ] becomes [ | val | val := nil. .... ].
>

It should only do this for inlined blocks that are in loops (to:do:,
to:by:do: whileTrue: whileTrue whileFalse whileFalse:).  I would modify the
decompiler to strip the val := nil, since it is implicit; all temps are
implicitly initialized to nil on entry to their scope.  The compiler inserts
the val := nil assignments to achieve this for blocks inlined in loops.  If
it didn't a subsequent enumeration of the block could see the value of a
local from a previous iteration.

e.g.

1 to: 10 do: [:i| | local | self assert: local isNil. local := i]

There is a test for this in the Compiler tests somewhere.


> And there is this snippet of code:
>
> | aBlock |
> aBlock := Compiler evaluate: '| x y |  [:a :b | x := a. y := b. x+y]'.
> aBlock decompile
>
> which breaks an assertion in the Decompiler itself.
>
> So this is far from complete.
>
>
>>
>>>  Right.  And there's now a visitor method for determining that scope,
>>>> TempScopeEditor>>#blockNode:isEnclosingScopeFor:.  But I've been too
>>>> lazy^H^H^H^Hbusy to apply it in the context of the decompiler.  Feel free to
>>>> feel motivated...
>>>>
>>>
>>> No, no, no, no, no, no, no.  The visitor is VariableScopeFinder and the
>>> method is ofVariable: so aParseNode accept: (VariableScopeFinder new
>>> ofVariable: var) answers nil or the smallest enclosing scope for var in
>>> aParseNode.  See Parser>>declareUndeclaredTemps.
>>>
>>
> The changeset contains the following changes:
>  - A new subclass of VariableScopeFinder: TempVariableScopeFinder. It finds
> scopes for tempvar nodes not just for undeclared nodes.
>  -
> In DecompilerConstructorForClosures>>#codeMethod:block:tempVars:primitive:class:
> instead of
>      [ block temporaries: temporaries ], we execute
>      [ temporaries := self pushDownTemps:
> temporaries toTheirInnermostPossibleScopesIn: block ]
>  - The method pushDownTemps:toTheirInnermostPossibleScopesIn: which tries
> to do what it's name suggests and returns with the temps remaining in the
> top level scope.
>
> Is this the right direction?
>

Yes!


>
> Cheers, Balázs
>

thank you!

best
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20110114/d891af0d/attachment.htm