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

[Segher Boessenkool]

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.

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.
> CREATE CELL * ALLOT		\ creates and initializes the instance

CELL *  is  CELLS   (which is a standard Forth word, CELL is not).
> 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.
> +; 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.
> +\ 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?
> +: 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.
> +: 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.
> +\ 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?
> +: getLocalRecordCount
> arrayCount 4 /
>
> +;

Oh.  Use a struct?

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.

Segher