[ZooLib-dev] Moderate ZStream/ZStrim revamp in the pipeline

[Andrew G. <ag...@em...>]

About three years ago we redesigned ZooLib's streams. There was some  
rationalization of names, using the short suffixes R, W, RW and RWPos  
rather than Input, Output, IO and Extent. And the hierarchy was  
subsequently augmented with the positionable read and write streams  
ZStreamRPos and ZStreamWPos and the un-readable ZStreamU. The  
fundamental design has otherwise not changed in the interim and has  
served ZooLib well.

By defining ZStreamR and ZStreamW as independent interfaces, and doing  
away with their precursor's involvement in lifetime issues, it's become  
straightforward to create and use a wide range of filter streams --  
streams that take another stream as a source or sink and transform the  
data read or written.

For example, if your code has been passed a reference to a ZStreamR  
that you know contains base64-encoded data you can wrap a  
ZStreamR_Base64Decode around it and pass that to code that wants  
whatever's 'inside' the base64 data stream:

   ZStreamR_Base64Decode theSB64(iStreamR);
   OtherFunction(theSB64).

Another useful situation is when you have a stream whose Imp_Write  
implementation has high latency but you need to make many calls to it.  
Wrapping a ZStreamW_Buffered around it reduces the number of calls that  
have to be made, and thus the time wasted in latency:

   ZStreamW_Buffered theSB(iStreamW);
   OtherFunctionMakingManyCalls(theSB);

However, if we were doing something like the following:

   ZStreamWPos_HighLatency theStreamWPos;
   ZStreamW_Buffered theSB(theStreamWPos);
   OtherFunctionMakingManyCalls(theSB);
   size_t thePosition = theStreamWPos.GetPosition();

thePosition would not have the value we might expect. The problem in  
this situation is that after OtherFunctionMakingManyCalls() returns  
theSB will likely still have data in its buffer that has not been  
passed on to theStreamWPos. It will pass the data on when it's  
destroyed, but here it's still in scope. One solution would be to do  
this:

   theSB.Flush();
   size_t thePosition = theStreamWPos.GetPosition();

but the Flush will be passed on to theStreamWPos, and  
ZStreamWPos_HighLatency's Flush implementation might suffer from truly  
awful latency. The ideal would be as follows:

   ZStreamWPos_HighLatency theStreamWPos;
   OtherFunctionMakingManyCalls(ZStreamW_Buffered(theStreamWPos));
   size_t thePosition = theStreamWPos.GetPosition();

Here the lifetime of the buffered stream extends from just before we  
invoke OtherFunctionMakingManyCalls to just after it returns. Its  
destructor will pass buffered data on to the sink stream (without  
flushing), and thePosition will have the value we'd expect.

Part of the goal for the stream redesign was to support exactly this  
kind of thing -- wrapping a temporary filter stream around some other  
stream. All extant ZooLib code that took a ZStreamR or ZStreamW had it  
passed as a non-const reference. Unfortunately, coincidental with the  
ZStream revamp, CodeWarrior tightened its conformity to the C++ spec.  
The C++ spec objects to the passing of a temporary object, like our  
ZStreamW_Buffered, via a non-const reference. This constraint flags  
situations where a conversion operator has, possibly unexpectedly,  
created a temporary object which will be mutated rather than the object  
one was expecting to be affected. In our situation it's safe, but the  
compiler can't know that. This can be worked around by old-style  
casting the stream thus:

    
OtherFunctionMakingManyCalls((ZStreamW&)ZStreamW_Buffered(theStreamWPos) 
);

but that's pretty heinous, particularly when you want to be able to  
chain several filters together:

    
OtherFunctionMakingManyCalls(ZStreamW_Base64Encode(ZStreamW_Buffered(the 
StreamWPos)));

becomes

    
OtherFunctionMakingManyCalls((ZStreamW&)ZStreamW_Base64Encode((ZStreamW& 
)ZStreamW_Buffered(theStreamWPos)));

ugh!

The solution would require two changes. Firstly, any function taking a  
stream that could benefit from having such a filter passed to it in  
lieu of a 'real' stream would have its signature modified to take a  
const reference to a stream. That includes the constructors for filter  
streams themselves. Secondly, in order that a function being passed a  
const reference be able to invoke methods on that reference those  
methods would need to be qualified as being const.

I've been reluctant to implement these changes as it seems to violate  
const-correctness. But I haven't come across any situation where  
maintaining the constness of a stream would actually be meaningful -- a  
read or write stream has no real state beyond the data it could return  
or had consumed, and thus a const stream was essentially useless.  
Having become entirely fed up with not being able to effectively use  
this chained-stream idiom I've given in and made the changes.

The consequence to application code is minimal. Any extant code that  
passes a stream to ZooLib will see no difference. And any code that's  
already passing around non-const streams will similarly not be affected.

However, in order to flush out potential problems, and to make more  
distinct the difference between the stream API that's expected to be  
used versus that which must be implemented in a new stream, I've made  
some name changes to the implementation API. Previously we had ReadImp,  
GetCountReadable, WriteImp and Flush. ReadImp has become Imp_Read,  
WriteImp has become Imp_Write, and GetCountReadable and Flush are now  
inlines that call through to the virtual functions Imp_CountReadable  
and Imp_Flush. These two are actually the ones that might cause you  
problems if you've overridden them. However your compiler will kick out  
a warning that your GetCountReadable and Flush virtual methods hide the  
same names in the base class -- this is the indication that you need to  
change those method names.

There are some more changes that seemed appropriate at this time,  
including making const the methods of ZStreamU, ZStreamRPos,  
ZStreamWPos and ZStreamRWPos. In overrides of these classes you'll need  
to rename your Unread, GetPosition, SetPosition, GetSize and SetSize  
methods to have an Imp_ prefix. You'll likely be told both that your  
name hides one in a base class *and* that you can't instantiate your  
subclass because of unimplemented pure virtual Imp_ prefixed methods.

The other significant change may seem spurious, but is crucial in  
enabling the use of the chained filters idiom. ZooLib has a pretty  
consistent style for parameters. We use 'i', 'io' and 'o' prefixes to  
indicate that a parameter is an input, an input/output or an output.  
Inputs come first in the parameter list, followed by I/O and then  
outputs. Something like a stream does not really fall under any of the  
categories. In itself it's not really an input or an output, it's  
something that's 'used' by the function. It seems a little too extreme  
to standardize a 'u' prefix, and so I've used 'i'. ZooLib's methods  
taking a stream (read or write) generally do so as the first parameter,  
keeping it from getting mixed up with other parameters that qualify  
what should be done with the stream. And filter stream constructors  
have followed that convention.

The problem is that when one chains together filters requiring  
parameters other than the source or sink stream the code reads rather  
like German in reverse. For example, ZStreamW_Buffered takes not just a  
sink stream (as in the earlier examples) but also a buffer size. And  
ZStreamW_CRLFRemove, which removes CR/LF byte pairs from the written  
data, takes a parameter being the byte to substitute for CR/LF. The  
CR/LF removal is most efficient when it can scan a large buffer for the  
sequence, so to improve its efficiency by accumulating data in a buffer  
one could do the following:

    
FunctionTakingStream(ZStreamW_Buffered(ZStreamW_CRLFRemove(theStreamW,  
'\n') , 1024));

See what I mean? Whereas a German sentence has qualifiers up front with  
the verb trailing we have the converse -- the parameters to the  
outermost filter are as far from it as possible, and it becomes hard to  
parse what's going on. Let's make matters worse by level 3 zlib  
compressing the CRLF-removed data before passing it on:

    
FTS(ZStreamW_Buffered(ZStreamW_CRLFRemove(ZStreamW_ZLibEncode(theStreamW 
, 3), '\n') , 1024));

Double ugh! At least the solution is simple. Filter stream constructors  
should take their sink or source stream as their *last* parameter. When  
we do this our examples become:

   FunctionTakingStream(ZStreamW_Buffered(1024,  
ZStreamW_CRLFRemove('\n', theStreamW)));
   FTS(ZStreamW_Buffered(1024, ZStreamW_CRLFRemove('\n',  
ZStreamW_ZLibEncode(3, theStreamW))));

Granted there are lots of parentheses at the end, but it's  
substantially more intelligible.

This moving of the stream parameter *only affects filter stream  
constructors*. I repeat, ZooLib still generally has the ZStreamR or  
ZStreamW parameter to functions in the first position, although they  
are now const references.

I've made similar changes to ZStrimXXX, and copied the constructor  
parameter order changes to ZStreamer and ZStrimmer.

Let me know if this sounds like it will cause significant grief for  
anyone.

A+
-- 
Andrew Green                           mailto:ag...@em...
Electric Magic Co.           Vox/Fax: +1 (408) 907 2101