[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