[OpenBIOS] [PATCH 7/10] Apple local variables implementation in Forth

[Segher Boessenkool]

>> Segher - as you seem to be one of the most fluent people in Forth  
>> out of everyone on the list - would you be willing/able to review  
>> this particular patch?
>
> I would prefer to ignore anything to do with local variables :-P
>
> But, if you insist, I can take a look.  Only problem is I deleted
> the mail with the patch already, so please forward it to me, or
> resend it.

Mark sent me a copy, thanks: replying to his comments in that:

> Here is a copy of the local words patch for review. From my  
> perspective, the things I have noticed:
>
> - I would prefer the locals code to be held within a separate file,  
> locals.fs

Ack.

> - "defer spin" is entirely unrelated to this patch. If PPC-specific  
> it should be moved into arch/ppc/qemu/tree.fs with an appropriate  
> comment

Yeah.

> - Some variable names use underscores "_" instead of "-" which  
> appears to be the normal Forth convention

Underscore was not part of the character set of most machines at the  
time
the first Forth systems were built.  Most Forth programmers find dashes
more readable than underscores.  Consistency is good.

> - Perhaps some more comments and an example included as a comment  
> at the top of the file?

Yeah, would be good if the syntax was described.


> +: has-locals ( -- true/false )
> +  skipws
> +  ib >in @ + c@ 7b <>
> +  if false exit then
> +  ib >in @ + 1+ c@ 20 >
> +  if false exit then
> +
> +  true
> + ;

Ugh.

: {    ( .... ) ; IMMEDIATE

> +: comp-str-char ( str len char -- true/false )
> +  swap 1 <> if 2drop false exit then
> +  swap c@ <> if false exit then
> +  true
> + ;

Ugly, just do a string compare with a single-char string if you really
need this.  It's probably faster even :-P

> +variable locals_wordlist
> +variable locals_state \ 0 - reading args
> +                      \ 1 - ;
> +                      \ 2 - reading vars
> +: read-locals ( -- addr0 .. addrN addrCount )
> +  has-locals
> +  not if 0 false exit then \ no locals
> +
> +  0 locals_state !
> +  0 >r
> +
> +  s" get-current" $find drop execute

Why not run it directly?

> +  s" wordlist" $find drop execute

Same.  Also, you never destroy these wordlists again, they can
take quite a lot of memory.

> +  dup locals_wordlist !
> +  s" set-current" $find drop execute
> +
> +  parse-word 2drop \ ditch the {
> +
> +  begin
> +    parse-word
> +
> +    2dup 3b comp-str-char \ check for ;

Instead of 3b, write   [char] ;
But, like I said, something like   2dup s" ;" compare   is much
more readable.

> +    if 1 locals_state ! then
> +
> +    2dup 7d comp-str-char \ check for }
> +    not

Never use "not", use "0=" or "invert" (whichever is appropriate).

> +  while
> +    locals_state @ 1 <> if \ the ; is not a local variable so  
> ignore it
> +      header
> +
> +      locals_state @ 0= if \ only save the address if it is an arg
> +        r>
> +        here na1+ >r
> +        1+ >r
> +      then

Wow this word is huge, break it apart?

It is more common to write "cell+", not "na1+".

I cannot really follow the logic here, maybe it will become
easier to understand once you factor it.

> +
> +      3 , 0 ,
> +      reveal
> +    else \ if we hit a ; then move to next state
> +      2drop
> +      2 locals_state !
> +    then
> +  repeat
> +
> +  2drop
> +
> +  s" set-current" $find drop execute
> +
> +  r> 0
> +  begin
> +    2dup
> + >
> +  while
> +    r> -rot
> +    1+
> +  repeat
> +
> +  drop
> +
> +  true
> + ;
> +
> +: begin-locals ( addr0 .. addrN count hasLocals -- )
> +  not if drop exit then
> +
> +  dup 0> if
> +    0 do
> +      ['] (lit) , , ['] ! ,

This line should be something like

literal postpone !

if you write ?DO you can get rid of the conditional around this loop.

> +    loop
> +  else drop then
> +
> +  s" get-order" $find drop execute
> +  locals_wordlist @
> +  swap 1+
> +  s" set-order" $find drop execute
> + ;
> +
> +: end-locals ( -- )
> +  locals_wordlist @ 0= if exit then
> +
> +  0 locals_wordlist !
> +
> +  s" get-order" $find drop execute
> +  swap drop 1-
> +  s" set-order" $find drop execute
> + ;
> +
> +: -> parse-word $find drop na1+
> +  ['] (lit) , , ['] ! ,
> + ; immediate
> +
> +\
> +\ 7.3.9.1 Defining words
> +\
> +
>   : :
> -  parse-word header
> -  1 , ]
> +  parse-word >r >r
> +  read-locals
> +  r> r> header
> +  1 ,
> +  begin-locals
> +  ]
>     ;

Ugh.  If you move stuff to a  {  word, you can do the locals handling
from inside (FIND) (or similar) instead.

>   : :noname
> @@ -1426,6 +1551,7 @@
>     ;
>   : ;
> +  end-locals
>     ['] (semis) , reveal ['] [ execute
>     ; immediate



You do not handle locals in FCode, and your implementation does not fit
that very well.  Maybe it can be made to work, but maybe not (or not
easily).

In Apple's implementation, only eight locals are allowed.  The names
of the locals are stored in a fixed table of eight strings; (FIND)
looks at those before it looks at the search order.  The compiled
code refers to the locals by slot number (0 to 7); a word that uses
locals starts with an xt that a) creates a new locals frame; and b)
moves as many items as required from the data stack to the locals
frame; and c) arranges for the locals frame cleanup code to be run
on exit from this word (by pushing its xt to the return stack).

Not ideal either, but much less invasive.


Segher