[OpenBIOS] [PATCH 2/3] interpreter.fs: don't clobber stack across evaluate strings split on newlines

[Mark Cave-Ayland]

On 17/07/16 17:39, Segher Boessenkool wrote:

> On Sun, Jul 17, 2016 at 03:20:58PM +0100, Mark Cave-Ayland wrote:
>> When an evaluate string is split across a newline, the current string
>> position is assumed to be the top-most item on the stack. However
>> in the case of yaboot, items are left on the stack to be used by a
>> subsequent line within the same evaluate statement and so subsequent
>> lines are parsed incorrectly.
> 
> Nice catch!  These things are hard to debug.

Yeah. Tell me about it! ;)

>> --- a/forth/bootstrap/interpreter.fs
>> +++ b/forth/bootstrap/interpreter.fs
>> @@ -165,7 +165,9 @@ defer outer-interpreter
>>    over + over do 
>>      i c@ 0a = if 
>>        i over - 
>> +      rot >r
>>        (evaluate)
>> +      r>
>>        i 1+ 
>>      then 
>>    loop 
> 
> This becomes hard to read.  So you have (inside the "if"):
> 
> ( a b )        i over -
> ( a b i-b )    rot >r
> ( b i-b R: a ) (evaluate)
> ( R: a )       r>
> ( a )
> 
> swap >r dup i swap - (evaluate) r>
> 
> but that is even longer.  Wow.  Long definitions are bad, mkay? ;-)
> 
> It might be better without the DO loop (it blocks the R stack).  Probably
> it is best to break out a natural factor (like, find the length of a string
> until the first line break -- isn't there a word like it already, like
> "left-split" but splitting on any line break char instead of on one delim).

Actually I did experiment without the do loop, but like you I found that
the slight ease of the code within the if didn't seem to justify the
complexity outside of it which is why I decided to stick with what was
there.

The other reason, of course, is that the code is fairly well tested in
its current form and so if we want to get this in for 2.7 I'd much
prefer to minimise the changes in this particular patchset :)

It seems that left-split does exist in OpenBIOS, but according to the
IEEE1275 standard it takes only a single character to split upon so I'm
not convinced it would make things more sane in this particular case
where we want to split on more than one character.


ATB,

Mark.