On 2012-Nov-10 16:53:10 -0800, Devin Teske <devin.te...@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)
...
>@@ -185,21 +240,21 @@ 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)
>@@ -184,18 +223,15 @@ 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]?
>@@ -227,36 +263,26 @@ 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.
>@@ -329,19 +340,18 @@ 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.
>@@ -357,14 +367,14 @@ 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.
>@@ -521,17 +528,20 @@ create init_text8 255 allot
>
> \ If this is the ACPI menu option, act accordingly.
> dup menuacpi @ = 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 )
>@@ -950,100 +914,43 @@ 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.
pgpjhm7HlFkWe.pgp
Description: PGP signature
