On Tue, May 1, 2012 at 9:29 PM, Eliot Miranda <[email protected]>wrote:
> > > On Tue, May 1, 2012 at 9:13 AM, Mariano Martinez Peck < > [email protected]> wrote: > >> >> >> On Tue, May 1, 2012 at 6:09 PM, Mariano Martinez Peck < >> [email protected]> wrote: >> >>> Hi guys. I noticed stef did this issue: >>> http://code.google.com/p/pharo/issues/detail?id=5642 >>> However, now I have the following test that fails in Pharo 2.0 but works >>> fine in 1.4: >>> >>> | string context | >>> string := 'test'. >>> context := [self class. string asUppercase] asContext. >>> self assert: (context tempNamed: 'string') = 'test' >>> >>> the current implementation of #tempNamed: is: >>> >>> tempNamed: aName >>> "Returns the value of the temporaries, aName." >>> "Implementation notes: temporary initialization in blocks simply >>> uses pushNil to allocate and initialize each temp. So if one inspects >>> [|a|a:=2] and sends it self method symbolic you get: >>> >>> 13 <8F 00 00 05> closureNumCopied: 0 numArgs: 0 bytes 17 to 21 >>> 17 <73> pushConstant: nil >>> 18 <77> pushConstant: 2 >>> 19 <81 40> storeIntoTemp: 0 >>> 21 <7D> blockReturn >>> 22 <7C> returnTop >>> >>> And when we check self asContext pc we get 17, which is *before* the >>> nil is pushed. Therefore we should pay attention when querying a temporary >>> if the temporary allocation was executed." >>> >>> | index | >>> index := (self tempNames indexOf: aName). >>> ^ index >= stackp >>> >> >> >> Maybe the solution is to use #> rather than #>= ? >> > > right. > Ok, so I will integrate that as a first version. > But tempNames is fundamentally broken for closures. > Just to avoid confusing, #tempNames itself looks correct: tempNames "Answer a SequenceableCollection of the names of the receiver's temporary variables, which are strings." ^ self debuggerMap tempNamesForContext: self > It does not answer temps in indirection vectors. That is the whole point > of schematicTempNamesString; it gives the topology of temps in a method. > e.g. (where => means printIt returns...) > > (Collection>>#inject:into:) methodNode schematicTempNamesString => > 'thisValue binaryBlock (nextValue)[each binaryBlock (nextValue)]' > > This says that > a) at method level there are three temps, thisValue, binaryBlock and an > indirection vector, and in the indirection vector is one temp named > nextValue. > b) in the block in inject:into: there are three temps, each (the > argument), binaryBlock and an indirection vector, and in that indirection > vector is a temp named nextValue. > > This is all orchestrated by DebuggerMethodMap, so that > > aContext method debuggerMap tempNamesForContext: aContext > So #tempNames implementation is correct? > > answers a list of the flattened temp names in a context (flattening out > indirection vectors) and for the above would answer either #('thisValue' > 'binaryBlock' 'nextValue') or #('each' 'binaryBlock' 'nextValue'), and > > | map | > map := aContext method debuggerMap. > map namedTempAt: ((map tempNamesForContext: aContext) indexOf: > aTempName) in: aContext > > gets the temp from the unflattened temps in a context. This is how the > debugger accesses temp names. > > So you need to throw away the broken tempName: implementation and use > DebuggerMethodMap or some parse over schematicTempNamesString, because > *with closures temps are not simply at contiguous offsets on the stack*. > Make sense? > Actually, we do have: namedTempAt: index "Answer the value of the temp at index in the receiver's sequence of tempNames." ^self debuggerMap namedTempAt: index in: self and namedTempAt: index put: aValue "Set the value of the temp at index in the receiver's sequence of tempNames. (Note that if the value is a copied value it is also set out along the lexical chain, but alas not in along the lexical chain.)." ^self debuggerMap namedTempAt: index put: aValue in: self so, if I understand correctly all we need to do is to fix tempNamed: and tempNamedPut: so that the delegate to namedTempAt: and namedTempPut: rather than to tempAt: and tempAtPut: ? > > Now, one good way to understand this is through examples. inject:into: is > the canonical simple example I've used for years. But we can find more > complex examples that may help. So this query looks for all methods that > contain a bytecode to create an indirection vector with 2 or more elements: > > SystemNavigation new browseAllSelect: > [:m| | is | > is := InstructionStream on: m. > is scanFor: [:b| b = 138 and: [is followingByte between: 2 and: 127]]] > > and the simplest example in a trunk 4.3 image I find is: > > Bag>>sum > "Faster than the superclass implementation when you hold many instances of > the same value (which you probably do, otherwise you wouldn't be using a > Bag)." > | sum first | > first := true. > contents keysAndValuesDo: [ :value :count | > first > ifTrue: [ sum := value * count. first := false ] > ifFalse: [ sum := sum + (value * count) ] ]. > first ifTrue: [ self errorEmptyCollection ]. > ^sum > > which needs a two-element indirection vector because the block [ :value > :count |...] assigns to both sum and first. Hence > > (Bag>>#sum) methodNode schematicTempNamesString => '(sum > first)[value count (sum first)]' > > So in an activation of Bag>>sum the value of first is (sumContext tempAt: > 1) at: 2 (cuz tempAt: 1 is the indirection vector represented as (sum > first) in schematic temps, and first is the second element. But in an > activation of the block in Bag>>sum the value of first is (sumBlockContext > tempAt: 3) at: 2. > > > I can keep repeating myself until I'm blue in the face, but y'all have to > put in the effort to understand this simple scheme. Temps that are > modified after they are closed-over end up in indirection vectors. > > Thanks, I am trying to undersand and fix it :) > >> >> >>> ifTrue: [ nil] >>> ifFalse: [self tempAt: (self tempNames indexOf: aName)] >>> >>> >>> and previously it was: >>> >>> tempNamed: aName >>> ^self tempAt: (self tempNames indexOf: aName) >>> >>> >>> ideas? >>> >>> >>> -- >>> Mariano >>> http://marianopeck.wordpress.com >>> >>> >> >> >> -- >> Mariano >> http://marianopeck.wordpress.com >> >> > > > -- > best, > Eliot > > > > > -- Mariano http://marianopeck.wordpress.com
