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

[Programmingkid]

On Aug 27, 2012, at 4:44 PM, openbios-request at openbios.org wrote:

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openbios.org/pipermail/openbios/attachments/20120827/45495747/attachment-0001.html>