Message: 4
Date: Mon, 27 Aug 2012 22:35:40 +0200
From: Segher Boessenkool <segher@kernel.crashing.org>
To: The OpenBIOS Mailinglist <openbios@openbios.org>
Subject: Re: [OpenBIOS] [PATCH] Adds local variable support to
OpenBIOS.
Message-ID: <F74BFA60-0824-49A9-B592-D6862E0ACFB7@kernel.crashing.org>
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