[Scummvm-devel] Major "bug" / misfeature in the Stream system

[Max Horn]

Hi folks,

just a brief note to all: We discovered recently that our Stream  
system (including class File, the MemoryStreams, etc.) has a problem  
in that it never was clearly specified how the eos() (end-of-stream)  
and ioFailed() methods should work; in particular, when exactly they  
should return true/false, and when those values get reset.

Turns out that File works similiar to how C's stdio works: The EOF/EOS  
flag only gets set after you tried reading data *beyond* the end of  
the file. That is, after reading the last byte of the file, EOS is not  
signaled; only if you then try to read on.
In contrast to that, MemoryReadStream, SubReadStream etc. would signal  
EOS immediately once the last byte was read.

So far, this didn't really cause issues, but it became very apparent  
when I recently worked on our SeekableReadStream::readLine code, which  
relies on the precise behavior of eos()...

In fact, lots of our code doesn't ever check for eos() -- rather, it  
checkes for ioFailed exclusively. Which works for File objects, since  
those return true in ioFailed at EOF, too -- which MemoryReadStream  
does *not*, it just always returns false in ioFailed. Ooops...

Anyway, this is a bad thing for both people who want to write Stream  
objects as well as for those who want to use them. So, I think we need  
to do something about this. Here's what I propose:

1) Decide what shall be considered "correct" behavior from now on. My  
proposal: Let's do it exactly as stdio does. The advantage is that in  
stdio, the behavior of ferror/feof is thoroughly defined and well- 
understood by people. That is: EOS is only signaled after trying to  
read *beyond* the EOS (so File is "correct", but MemoryStream etc.  
need to introduce an internal EOS flag).
Also, add a new method err() / ioErr() or so, which is modelled after  
ferror.
Keep the current ioFailed() as an alias for "eos() || ioErr()" to ease  
transition of existing code.
Add a new clearIoStatus() (or so) method which resets both the error  
and the EOS flag (like clearerr() in stdio); the current clearIoFailed  
will just call through to that.

An alternative idea would be to model after C++'s iostream; but I am  
not sure that would benefit us.


2) Write unit tests for all stream classes to verify the "correct"  
behavior (even if the code does not behave correctly yet ;)

3) Fix / enhance all Stream classes as needed

4) Go through all client code (mostly engines) to verify they still  
work fine. try to replace ioFailed where possible, so we can get rid  
of it.


Does that sounds like a reasonable plan? Any volunteers to help with  
it? Because I am pretty swamped with other stuff :).

Cheers,
Max