[Vm-dev] Request for review of VMMaker-dtl.262.mcz

[David T. Lewis]

On Thu, Jan 05, 2012 at 03:56:35PM +0100, Stefan Marr wrote:
> 
> Hi Dave:
> 
> On 05 Jan 2012, at 15:31, David T. Lewis wrote:
> 
> > With respect to the arguments passed to the primitive on the
> > stack, you can see (I hope) what they should be by looking at
> > the patch below, since I put the stack operations in this one
> > method. In "normal" use, there are three arguments on the stack,
> > and when invoked as a mirror primitive, there is a fourth
> > argument the specifies the object being mirrored.
> 
> Well, ok. I see now where my assumption was wrong.
> I thought #primitivePerformInSuperclass was implemented in terms of #primitivePerform.
> But apparently it is #primitivePerformAt: (being the common part of #primitivePerformInSuperclass and #primitivePerformWithArgs).
> 
> *sigh* 
> 
> While you are at it, could you improve the naming of #primitivePerformAt: and rename it to #primitivePerformWithArgsAt:?
> 
> That is certainly the name it should have had. And since it is only a helper function as far as I can tell it should not cause to much problems.
> 
> Thanks
> Stefan

At this point I prefer not to change anything in this area unless
there is a specific reason to do so. My first priority is to not
break anything, second priority is to make the mirror primitives
work, third priority is to make it consistent with Cog where I can,
and last priority would be cosmetic changes or method renaming.
Better method comments would not be out of the question though.

So if I sound overly conservative here, but we are dealing with a
part of the VM that was designed and implemented by Dan Ingalls,
and I am just an amateur VM hacker trying not to break it ;-)

Cheers,
Dave

> 
> 
> > 
> > Dave
> > 
> > On Thu, Jan 05, 2012 at 03:18:58PM +0100, Stefan Marr wrote:
> >> 
> >> Hi David,
> >> Hi Eliot (this might related to my problem from yesterday):
> >> 
> >> Do you remember the discussion of primitivePerform and primitivePerformInSuperclass we had 8th to 12th December?
> >> 
> >> One implicit assumption, based on the implementation, I made was that I do not have to give an array of arguments to the primitives. It would just take the arguments from the stack and at least to my understanding, would do the right thing.
> >> 
> >> Your new implementation does not seem to do that anymore.
> >> 
> >> I am not sure whether that is the problem I am seeing with Cog right now (reported yesterday).
> >> But your implementation looks like it breaks my code :(
> >> 
> >> What is the correct semantics for primitivePerform and primitivePerformInSuperclass with regard to arguments?
> >> Is it really not supported to provide them on the stack?
> >> 
> >> Thanks
> >> Stefan
> >> 
> >> On 05 Jan 2012, at 14:51, David T. Lewis wrote:
> >> 
> >> 
> >>>> 
> >>>> 
> >>>> Reference Mantis 7429: Add Mirror Primitives to the VM
> >>>> 
> >>>> Update primitivePerformInSuperclass to support ContextPart>>object:perform:withArguments:inClass:
> >>>> 
> >>>> Implementation differs from that of oscog in that the original primitivePerformAt: is retained unmodified, and the necessary stack adjustments are done in primitivePerformInSuperclass for the special case of argumentCount 4 (mirror primitive call) rather than 3. The oscog approach may be prefered (not least for its clearer method naming), but making the change in primitivePerformInSuperclass is low risk and more easily implemented by a Sunday Squeaker.
> >>>> 
> >>>> All MirrorPrimitiveTests pass.
> >>>> 
> >>>> =============== Diff against VMMaker-dtl.261 ===============
> >>>> 
> >>>> Item was changed:
> >>>> ----- Method: Interpreter>>primitivePerformInSuperclass (in category 'control primitives') -----
> >>>> primitivePerformInSuperclass
> >>>> 	| lookupClass rcvr currentClass |
> >>>> 	lookupClass := self stackTop.
> >>>> + 	rcvr := self stackValue: 3.
> >>>> - 	rcvr := self stackValue: argumentCount.
> >>>> 	currentClass := self fetchClassOf: rcvr.
> >>>> 	[currentClass ~= lookupClass]
> >>>> 		whileTrue:
> >>>> 		[currentClass := self superclassOf: currentClass.
> >>>> 		currentClass = nilObj ifTrue: [^ self primitiveFail]].
> >>>> 
> >>>> + 	argumentCount = 3
> >>>> + 		ifTrue: ["normal primitive call with 3 arguments expected on the stack"
> >>>> + 			self popStack.
> >>>> + 			self primitivePerformAt: lookupClass.
> >>>> + 			self successful ifFalse:
> >>>> + 				[self push: lookupClass]]
> >>>> + 		ifFalse: ["mirror primitive call with extra argument specifying object to serve as receiver"
> >>>> + 			| s1 s2 s3 s4 s5 |
> >>>> + 			"save stack contents"
> >>>> + 			s1 := self popStack. "lookupClass"
> >>>> + 			s2 := self popStack. "args"
> >>>> + 			s3 := self popStack. "selector"
> >>>> + 			s4 := self popStack. "mirror receiver"
> >>>> + 			s5 := self popStack. "actual receiver"
> >>>> + 			"slide stack up one, omitting the actual receiver parameter"
> >>>> + 			self push: s4. "mirror receiver"
> >>>> + 			self push: s3. "selector"
> >>>> + 			self push: s2. "args"
> >>>> + 			"perform as if mirror receiver had been the actual receiver"
> >>>> + 			self primitivePerformAt: lookupClass.
> >>>> + 			self successful ifFalse:
> >>>> + 				["restore original stack"
> >>>> + 				self pop: 3. "args, selector, mirror receiver"
> >>>> + 				self push: s5. "actual receiver"
> >>>> + 				self push: s4. "mirror receiver"				
> >>>> + 				self push: s3. "selector"
> >>>> + 				self push: s2. "args"
> >>>> + 				self push: s1. "lookup class" ]]
> >>>> + !
> >>>> - 	self popStack.
> >>>> - 	self primitivePerformAt: lookupClass.
> >>>> - 	self successful ifFalse:
> >>>> - 		[self push: lookupClass]!
> >>>> 
> >>>> Item was changed:
> >>>> ----- Method: VMMaker class>>versionString (in category 'version testing') -----
> >>>> versionString
> >>>> 
> >>>> 	"VMMaker versionString"
> >>>> 
> >>>> + 	^'4.7.19'!
> >>>> - 	^'4.7.18'!
> >> 
> >> -- 
> >> Stefan Marr
> >> Software Languages Lab
> >> Vrije Universiteit Brussel
> >> Pleinlaan 2 / B-1050 Brussels / Belgium
> >> http://soft.vub.ac.be/~smarr
> >> Phone: +32 2 629 2974
> >> Fax:   +32 2 629 3525
> 
> 
> 
> -- 
> Stefan Marr
> Software Languages Lab
> Vrije Universiteit Brussel
> Pleinlaan 2 / B-1050 Brussels / Belgium
> http://soft.vub.ac.be/~smarr
> Phone: +32 2 629 2974
> Fax:   +32 2 629 3525