<br><br><div class="gmail_quote">On Fri, Jan 14, 2011 at 5:42 AM, Balázs Kósi <span dir="ltr"><<a href="mailto:rebmekop@gmail.com">rebmekop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="gmail_quote">Hi,</div><div class="gmail_quote"><div> </div><div>We fiddled with this a bit.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="gmail_quote"><div>On Thu, Jan 13, 2011 at 6:52 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On Thu, Jan 13, 2011 at 9:33 AM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On Thu, Jan 13, 2011 at 2:29 AM, Bert Freudenberg <span dir="ltr"><<a href="mailto:bert@freudenbergs.de" target="_blank">bert@freudenbergs.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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:<br>
<br>
blockLocalsAndArgs<br>
| product |<br>
product := 1.<br>
1 to: 2 do: [:i |<br>
i = 1<br>
ifTrue: [ | factor | factor := 6.<br>
product := product * factor ]<br>
ifFalse: [ #(9) do: [:factor |<br>
product := product * factor ] ] ].<br>
^ product = 13r42<br>
<br>
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.<br>
<br>
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</blockquote></div></div></blockquote>
</div></div></blockquote></div></div></div></blockquote><div><br></div></div><div> The Decompiler modified by the attached changeset tries to do this. It gives the following output for your example:</div><div class="im">
<div><br></div><div> blockLocalsAndArgs</div>
<div><span style="white-space:pre-wrap"> </span>| product |</div><div><span style="white-space:pre-wrap"> </span>product := 1.</div><div><span style="white-space:pre-wrap"> </span>1</div><div><span style="white-space:pre-wrap"> </span>to: 2</div>
<div><span style="white-space:pre-wrap"> </span>do: [:i | i = 1</div><div><span style="white-space:pre-wrap"> </span>ifTrue: [| factor |</div><div><span style="white-space:pre-wrap"> </span>factor := 6.</div><div>
<span style="white-space:pre-wrap"> </span>product := product * factor]</div><div><span style="white-space:pre-wrap"> </span>ifFalse: [#(9 )</div><div><span style="white-space:pre-wrap"> </span>do: [:factor | product := product * factor]]].</div>
</div><div><span style="white-space:pre-wrap"> </span>^ product = 54</div><div> </div><div>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. .... ].</div>
</div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>e.g.</div><div><br></div><div>1 to: 10 do: [:i| | local | self assert: local isNil. local := i]</div><div><br></div><div>There is a test for this in the Compiler tests somewhere.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="gmail_quote">
<div> </div><div>And there is this snippet of code:</div><div> </div><div>| aBlock |</div><div>aBlock := Compiler evaluate: '| x y | [:a :b | x := a. y := b. x+y]'.<span style="white-space:pre-wrap"> </span></div>
<div><span style="white-space:pre-wrap"></span>aBlock decompile</div><div> </div><div>which breaks an assertion in the Decompiler itself.</div><div> </div><div>So this is far from complete.</div><div class="im"><div><br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>
</div><div>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...</div>
</div></blockquote><div><br></div></div><div>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.</div>
</div></blockquote><div></div></div></div></div></blockquote><div> </div></div><div>The changeset contains the following changes:</div><div> - A new subclass of VariableScopeFinder: TempVariableScopeFinder. It finds scopes for tempvar nodes not just for undeclared nodes.</div>
<div> - In DecompilerConstructorForClosures>>#codeMethod:block:tempVars:primitive:class: instead of </div><div> [ block temporaries: temporaries ], we execute</div><div> [ temporaries := self pushDownTemps: temporaries toTheirInnermostPossibleScopesIn: block ]</div>
<div> - The method pushDownTemps:toTheirInnermostPossibleScopesIn: which tries to do what it's name suggests and returns with the temps remaining in the top level scope.</div><div> </div><div>Is this the right direction?</div>
</div></blockquote><div><br></div><div>Yes!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="gmail_quote">
<div> </div><div>Cheers, Balázs</div></div></blockquote><div><br></div><div>thank you!</div><div><br></div><div>best</div><div>Eliot </div></div><br>