On Aug 27, 2012, at 4:44 PM, openbios-request(a)openbios.org wrote:
Date: Mon, 27 Aug 2012 22:35:40 +0200
From: Segher Boessenkool <segher(a)kernel.crashing.org>
To: The OpenBIOS Mailinglist <openbios(a)openbios.org>
Subject: Re: [OpenBIOS] [PATCH] Adds local variable support to
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
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
Just kill this part.
I don't think ignoring all possible error situations is better than handing a few of
+ CREATE CELL * ALLOT \ creates and initializes
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
-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.
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 )
: 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
+ 0= if \ if there is no more text to parse
+ dup " ;" comp
+ 0= if \ if the semicolon is encountered
+ drop \ drop the duplicated address
+ " }" comp
+ 0= if \ if '}' character is encountered
+ true \ end loop because '}' marks end of local variables
+ variableCount 1 + TO variableCount
+ >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
+: allocateMemory ( n - addr )
+ alloc-mem dup 0= if
+ cr cr 10 spaces abort" Failed to allocate memory for local
variables!" cr cr
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
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.
+ arrayCount 4 /
Oh. Use a struct?
The little documentation on struct I found did not indicate it was a better replacement
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?