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

[Mark Cave-Ayland]

On 10/08/11 16:43, William Hahne wrote:
> Very interesting. I think this needs someone with quite strong
> Forth-fu (Stefan) to review this one. Also I can't see any related
> documentation with this patch related to locals support?
>
>
> This is an Apple specific thing. See
> http://www.openfirmware.info/How_Local_Variables_in_Forth_Work_---_Using_App...

Right I understand this, but I'm fairly sure there is an official spec 
for Forth local variables somewhere. For something to be committed, I'd 
expect the commit reference to contain a link to the source 
specification and comments in the code giving an indication as to what 
the level of implementation actually is (I absolutely understand that it 
can be impractical to implement everything within the time available).

ATB,

Mark.

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

[William Hahne]

On Mon, Aug 15, 2011 at 7:03 AM, Mark Cave-Ayland
 wrote:
> Right I understand this, but I'm fairly sure there is an official spec for
> Forth local variables somewhere. For something to be committed, I'd expect
> the commit reference to contain a link to the source specification and
> comments in the code giving an indication as to what the level of
> implementation actually is (I absolutely understand that it can be
> impractical to implement everything within the time available).

Forth itself has no local variables, there are several unofficial
implementations, but I am focusing on Apple's implementation. As far
as I can tell the implementation is complete, this is why there are no
comments noting the level of completeness.

William Hahne
> ATB,
>
> Mark.
>
> Sirius Labs: http://www.siriusit.co.uk/labs

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

[Segher Boessenkool]

> Right I understand this, but I'm fairly sure there is an official  
> spec for Forth local variables somewhere.

Standard (ISO/ANS) Forth (1994) has a LOCALS wordset, which is meant to
build more convenient syntaxes on, not to be used directly.  The various
locals wordsets people used at that time differed too much.

Nowadays implementations for locals have converged a bit more, and the
upcoming Forth 201x standard will most likely include a locals wordset
not unlike what Apple used, but slightly different syntax (and quite
different semantics if you look deeply!)

Gforth has similar syntax, as do many other systems.

And, of course, local variables in Forth is an abomination that should
be avoided -- just like ROLL etc. :-)

Segher

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

[Mark Cave-Ayland]

On 17/08/11 07:47, Segher Boessenkool wrote:
> > Right I understand this, but I'm fairly sure there is an official spec
> > for Forth local variables somewhere.
>
> Standard (ISO/ANS) Forth (1994) has a LOCALS wordset, which is meant to
> build more convenient syntaxes on, not to be used directly. The various
> locals wordsets people used at that time differed too much.
>
> Nowadays implementations for locals have converged a bit more, and the
> upcoming Forth 201x standard will most likely include a locals wordset
> not unlike what Apple used, but slightly different syntax (and quite
> different semantics if you look deeply!)
>
> Gforth has similar syntax, as do many other systems.
>
> And, of course, local variables in Forth is an abomination that should
> be avoided -- just like ROLL etc. :-)

Now I understand. 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?

ATB,

Mark.

[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.

Segher

[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

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

[Segher Boessenkool]

> > 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.

Oh, and you're supposed to handle  |  exactly the same as  ;

Segher