Re: HEADS UP: Forth Optimizations

On 2012-Nov-10 16:53:10 -0800, Devin Teske <devin.teske_at_fisglobal.com> wrote:
>Can someone help review this for the commit log?

I've had a look through the proposed patch and my comments follow.
Other than that, it looks good to me.

>Index: menu-commands.4th
>===================================================================
>--- menu-commands.4th	(revision 242835)
>+++ menu-commands.4th	(working copy)
...
>_at__at_ -185,21 +240,21 _at__at_ variable root_state
...
> 	s" set kernel=${kernel_prefix}${kernel[N]}${kernel_suffix}"
>-	                          \ command to assemble full kernel-path
>-	-rot tuck 36 + c! swap    \ replace 'N' with array index value
>-	evaluate                  \ sets $kernel to full kernel-path
>+	36 +c! \ replace 'N' with ASCII numeral
>+	evaluate

I think the "sets $kernel to full kernel-path" comment is worth keeping.

> 	s" set root=${root_prefix}${root[N]}${root_suffix}"
>-	                          \ command to assemble root image-path
>-	-rot tuck 30 + c! swap    \ replace 'N' with array index value
>-	evaluate                  \ sets $kernel to full kernel-path
>+	30 +c! \ replace 'N' with ASCII numeral
>+	evaluate

Likewise, this could do with a (corrected) comment that it sets $root
to the full path to root.

>Index: menu.4th
>===================================================================
>--- menu.4th	(revision 242835)
>+++ menu.4th	(working copy)
>_at__at_ -184,18 +223,15 _at__at_ create init_text8 255 allot
> 
> 		\ base name of environment variable
> 		loader_color? if
>-			s" ansi_caption[x]"
>+			dup ansi_caption[x]
> 		else
>-			s" menu_caption[x]"
>+			dup menu_caption[x]
> 		then	

Could this be simplified to

= 		dup
= 		loader_color? if
=			ansi_caption[x]
= 		else
=			menu_caption[x]
= 		then	

Or, at a higher level, should this whole block be pulled into a new
word (along with similar words for toggled_{ansi,text}[x] and
{ansi,menu}_caption[x][y]?

>_at__at_ -227,36 +263,26 _at__at_ create init_text8 255 allot
...
> 		getenv dup -1 <> if
> 			\ Assign toggled text to menu caption

Some comments on stack contents around here would make it somewhat
easier to follow what is going on.

>_at__at_ -329,19 +340,18 _at__at_ create init_text8 255 allot
...
> 			\ This is highly unlikely to occur, but to make
> 			\ sure that things move along smoothly, allocate
> 			\ a temporary NULL string
> 
>+			drop ( getenv cruft )
> 			s" "
> 		then
> 	then

Is this the memory leak?  If so, can I suggest that this be commited
separately since it is a simple change and is distinct from the other
changes you are proposing.

>_at__at_ -357,14 +367,14 _at__at_ create init_text8 255 allot
> 	\ 
> 	\ Let's perform what we need to with the above.
> 
>-	\ base name of menuitem caption var
>+	\ Assign array value text to menu caption
>+	4 pick

According to the docementation just above this hunk, there are only 4
items on the stack, so "4 pick" seems wrong, though it is consistent
with my understanding of the old code.  The "2 pick [char] 0" you
added earlier seems to similarly be out-by-one, though consistent.

>_at__at_ -521,17 +528,20 _at__at_ create init_text8 255 allot
> 
> 		\ If this is the ACPI menu option, act accordingly.
> 		dup menuacpi _at_ = if
>-			acpimenuitem ( -- C-Addr/U | -1 )
>+			dup acpimenuitem ( n -- n n c-addr/u | n n -1 )
>+			dup -1 <> if
>+				13 +c! ( n n c-addr/u -- n ) \ replace 'x'

I think the stack here should be ( n n c-addr/u -- n c-addr/u )

>_at__at_ -950,100 +914,43 _at__at_ create init_text8 255 allot
> 
> 	49 \ Iterator start (loop range 49 to 56; ASCII '1' to '8')
> 	begin
>-		\ Unset variables in-order of appearance in menu.4th(8)

Does the order matter?  I notice you've changed it.