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
headerlocals_state @ 0= if \ only save the address if it is an argr>here na1+ >r1+ >rthen
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
2drop2 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