Date: Mon, 12 Nov 2012 12:30:00 +1100 From: Peter Jeremy <peter@rulingia.com> To: Devin Teske <dteske@freebsd.org> Cc: Adrian Chadd <adrian@freebsd.org>, freebsd-current@freebsd.org Subject: Re: HEADS UP: Forth Optimizations Message-ID: <20121112013000.GD5594@server.rulingia.com> In-Reply-To: <A0A46155-0969-459E-BCD6-4AA7B8B371BE@fisglobal.com> References: <A0A46155-0969-459E-BCD6-4AA7B8B371BE@fisglobal.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--i0/AhcQY5QxfSsSZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On 2012-Nov-10 16:53:10 -0800, Devin Teske <devin.teske@fisglobal.com> wrot=
e:
>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
>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>--- menu-commands.4th (revision 242835)
>+++ menu-commands.4th (working copy)
=2E..
>@@ -185,21 +240,21 @@ variable root_state
=2E..
> s" set kernel=3D${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=3D${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
>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>--- menu.4th (revision 242835)
>+++ menu.4th (working copy)
>@@ -184,18 +223,15 @@ create init_text8 255 allot
>=20
> \ 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=09
Could this be simplified to
=3D dup
=3D loader_color? if
=3D ansi_caption[x]
=3D else
=3D menu_caption[x]
=3D then=09
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
=2E..
> 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
=2E..
> \ This is highly unlikely to occur, but to make
> \ sure that things move along smoothly, allocate
> \ a temporary NULL string
>=20
>+ 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
> \=20
> \ Let's perform what we need to with the above.
>=20
>- \ 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
>=20
> \ If this is the ACPI menu option, act accordingly.
> dup menuacpi @ =3D 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
>=20
> 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.
--i0/AhcQY5QxfSsSZ
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iEYEARECAAYFAlCgURgACgkQ/opHv/APuIdTRACgiVYnFNPNRSzqfiA5zoXv8/3K
06wAn1/tM3gsHOwCoeRuRKp5SgE8uzsS
=hm9F
-----END PGP SIGNATURE-----
--i0/AhcQY5QxfSsSZ--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121112013000.GD5594>