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

Segher Boessenkool segher at kernel.crashing.org
Mon Aug 27 22:35:40 CEST 2012


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




More information about the OpenBIOS mailing list