Re: [OpenBIOS] [PATCH] Adds local variable support to OpenBIOS.

[Programmingkid]

On Aug 27, 2012, at 4:44 PM, [e-mail] wrote:
> Message: 4
> Date: Mon, 27 Aug 2012 22:35:40 +0200
> From: Segher Boessenkool 
> To: The OpenBIOS Mailinglist 
> Subject: Re: [OpenBIOS] [PATCH] Adds local variable support to
>    OpenBIOS.
> Message-ID: 
> Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed
>
> Let me comment on the code, independently of if I think using local
> var names is a good idea at all.
> > DEPTH 0= IF
> > CR ." Please specify an array size." CR
> > EXIT
> > THEN
>
> You shouldn't blindly continue when you detect an error; use ABORT
> instead, or ABORT" since you want to print something.

Thanks for catching this. It has been corrected.
> Don't check stack depth, it is useless.  It doesn't work when you
> forgot to pass a parameter but there is other stuff on the stack
> already (the common case).  Your check also doesn't work if there
> is a negative number of parameters on the stack (the next most
> common case...)
>
> Just kill this part.

I don't think ignoring all possible error situations is better than handing a few of them.
> > CREATE CELL * ALLOT		\ creates and initializes the instance
>
> CELL *  is  CELLS   (which is a standard Forth word, CELL is not).

Thanks for letting me know about CELL. I have changed them all to CELLS
> > DEPTH 2 < IF
> > CR ." Please specify an index number." CR
> > drop		\ removes the address of the array instance
> > -1 throw	\ stop normal execution after error
> > THEN
>
> -1 THROW   is   ABORT
> Same thing here, just kill this code.

The error handling makes the word more user friendly. Having it fail and leaving the user clueless why doesn't sound right.
> > +; immediate
>
> Defining words should not be immediate in most cases, including here.
> > +\ Declare the local-base-address VALUE
> > +0 VALUE local-base-address
> >
> > +\ returns the base address used for the local words
> > +: getBaseAddress ( - addr )
> > local-base-address
> >
> > +;
>
> : really-get-base-address  getBaseAddress ;
> : really-really-get-base-address  really-get-base-address ;
>
> And don't use CamelCase.

Is this some official naming convention, or your own taste?
> > +\ Sets the first local variable's value
> > +: Local0!	( x - )
> > 0 CELL * getBaseAddress + !
> >
> > +;
>
> You can do something smarter than this...
> > +\ Sets the twelfth local variable's value
> > +: Local11!	( x - )
> > 11 CELL * getBaseAddress + !
> >
> > +;
>
> ...why stop at 11?

As far as I know, Apple's code only uses two local variables at most. I figured 12 local variables per word should be enough.
> > +: calculateNeededMemory ( "char" - n )
> > 0 TO variableCount
> > > in @	\ keep track of where the pointer was
> > begin
> > parse-word
> > 0= if			\ if there is no more text to parse
> > 	drop
> > 	true
> > else
> > 	dup " ;" comp
> > 	0= if		\ if the semicolon is encountered
> > 		drop	\ drop the duplicated address
> > 		false
> > 	else
> > 		" }" comp
> > 		0= if		\ if '}' character is encountered
> > 			true	\ end loop because '}' marks end of local variables
> > 		else	
> > 			variableCount 1 + TO variableCount
> > 			false
> > 		then
> > 	then
> > then
> > until
> > > in ! \ reset the pointer
> > variableCount CELL *
> >
> > +;
>
> Factor this?  You shouldn't ever use a variable that is used as a temp
> inside a word.

I'm not sure why you suggest this. It is just such a pain having to deal with the stack.
> > +: allocateMemory	( n - addr )
> > alloc-mem dup 0= if
> > drop
> > cr cr 10 spaces abort" Failed to allocate memory for local  
> >
> > variables!" cr cr
> > then
> >
> > +;
>
> alloc-mem is way too slow to be used on every function call.  Statically
> allocate a locals stack, or put it on an existing stack, or something
> like that.

I agree. That is why it is only used at compile time.
> > +\ Declares the size of the local variable table
> > +48 CONSTANT localTableSize
> >
> > +\ Declare the local variable table
> > +localTableSize ARRAY localVariableTable
>
> You have 48 but can only access 12?

That is four fields per local variable: 48 / 4 = 12.
> > +: getLocalRecordCount
> > arrayCount 4 /
> >
> > +;
>
> Oh.  Use a struct?

The little documentation on struct I found did not indicate it was a better replacement for Array.
> I'll stop here.  There are two good ways to implement locals (or
> anything else, really): 1) Make it _simple_: you can do locals
> in about 20 or 30 lines of code.  60 is fine, too.  2) Make it
> efficient: don't allocate from a heap, don't run much code on
> the hot paths.  This will be much smaller code as well, but
> perhaps trickier to understand.

What is a hot path?
> Segher