On Fri, Jan 14, 2011 at 2:42 PM, 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. .... ].
>
> 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?
>
> Cheers, Balázs

I resend it in plain text, because
http://lists.squeakfoundation.org/pipermail/squeak-dev/2011-January/156794.html
says
"Skipped content of type multipart/alternative"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pushDownTemps.st
Type: application/octet-stream
Size: 3379 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20110114/6d01e738/pushDownTemps.obj