[boost] Re: IOStreams formal review start

From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-09-09 19:51:42

"Pavel Vozenilek" <pavel_vozenilek_at_[hidden]> wrote in message
news:chq3qp$pc4$2_at_sea.gmane.org...
>
> "Jonathan Turkanis" wrote:
>
> ____________________________________________________
> > Here's a suggestion for a concept which might be reusable
> > in this way (view with fixed-width font):
> >
> > Concept: Resettable
> >
> > Valid Expressions     | Return Type | Semantics
> > --------------------------------------------------------------------
> > t.reset(discard)      | bool        | resets t to the state it had
> >                       |             | upon construction, discarding
> >                       |             | cached data if discard is true.
> >                       |             | Returns true for success.
> >
> Counterexample: the radio link pipeline has
> member counting # of passed bytes, member that keeps
> ouput rate in sync with current air conditions.
>
> If you want discard data in pipeline, you would reset all
> these settings as side-effect.

I see.

> I have feeling there should be two concepts:
> - reset(bool discard)
> - discard()

> ____________________________________________________
> > So when I define the convenience base
> > classes, I want to make the i/o category refine openable,
> > closable and resettable, and provide default no-op implementations.
>
> > Now, if the user only implements reset, I'd like boost::io::open and
> > boost::io::close to call reset.
> >
> > ... variant with dummy instance + member function compare
> > ... variant with CRTP
> >
> I do not have clear picture: when io::open() is called the
> real type of filter is not known?

Right now, there's no function open. And in pronciple, there never needs to be
since acomponent can set an 'open' flag upon each i/o operation, and clear it
when close() is called. I'm thinking Openable might be a good addition, as a
convenience. If so, it would be called as soon as a chain becomes complete.

> > > - it is not really sure flush would flush: for example filter removing
> > >    certain word may wait intil words get finished. What is flush()
> > >    semantic of theirs?
> >
> > This is an important point. When I originally implemented Flushable,
flush()
> > was just a 'suggestion' to flush buffers. So users could never be sure there
> > wasn't any cached data remaining. I did it this way because for many
filters,
> > as you mentioned, you can't force a complete flush without violating data
integrity.
> > But this whimpy version of flush turned out not to be very useful, which is
> > partly why I eliminated it.
> >
> > If I restore the concept, I will probably have flush return true only if all
> > cached data could be safely sent downstream. This means that
> > sometimes flushing a filtering_ostream will fail, and in those cases
> > you won't be able to insert or remove filters while i/o is in progress.
> >
> Maybe bool flush(bool force = false).

> E.g. close() should probably call flush(true) to avoid data loss.
>
> If an filter decides individually its current data just cannot
> be flushed it could ignore the 'force' flag.
>
> The flush() returns true if flush was complete. User then may
> discard the rest of data sitting in caches.

Sounds good.
_______________________________________________
> > > > >  - halt temporarily/restart (for output streams only?).
> > > > >       This action may be propagated downstream since some
> > > > >      filters may have busy sub-threads of their own
> > > >
> > > > I'm not sure I understand.
> > > >
> > > Equivalent to putting null sink on the top of pipeline
> > > (and maybe after each part of pipeline, if the part acts
> > > asynchonously).
> > >
> > > Not sure now it it is worth the troubles.
> >
> > Neither am I ;-). Maybe if you can think of an important use case ...
> >
> >
> Only contrived: someone has reference to one and only one
> member of pipeline, not to initial data source or end data sink.
> This one may want to halt temporarily the flow but doesn't want
> any dependencies on the rest of application.
>
> E.g. module who keeps TCP/IP throughput limit and manages
> multiple opened sockets. User registers socket_streams
> and they could all be halted/restarted by the module.
>
> (Here the halt means stop reading/sending, not null-sink
> equivalent. The make-it-null-sink looks as yet another thing.)

I think this could be done currently just by keeping a reference to the filter.

I'm fairly confident we can work out reasonable semantics for these operations.
To me, the most important question is what should happen if not all the
components in a chain model the concept. I'm very nervous about assuming that a
no-op is a reasonable default behavior, so I'm inclined to say the operations
must fail in that case. But doesn't that, in effect, force people who want to
write reusable components to provide implementations for a long list of
operations?

________________________________________________
> > > > >  - generate string description of what is in stream chain
> > > > >     (like stream::dump() for debugging)
> > > >
> > > > The character sequences, or the filter/resource sequence?
> > > >
> > > The latter. E.g. for to print debug into on console.
> >
> > This is a good idea for a debug-mode feature, but I think it only
> > really makes sense if I implement flushable and allow filters to
> > be swapped in and out during i/o. Otherwise, it should be pretty
> > obvious which filters are in a chain just by looking the sequence
> > of pushes.
> >
> And good idea for maintenace programmer.

True.

> ____________________________________________________
> Few notes from reading sources:
>
> 1. scope_guard.hpp: maybe this file could be moved into
>     boost/detail and used by multi_index + iostreams until
>    something gets boostified.

I'm for this. Unfortunately, my simplified scope_guard isn't working on CW8.3
(though it passes the regression tests I've written for it.) So I'll probably
use Joaquín's.

> 2. utility/select_by_size.hpp: would it be possible to use
>     PP local iteration here to make it bit simpler?

Probably. I didn't know about local iteration when I wrote it ;-)

> 3. io/zlib.hpp:
>
> #ifdef BOOST_MSVC
> # pragma warning(push)
> # pragma warning(disable:4251 4231 4660)         // Dependencies not
> exported.
> #endif
>
> could be rather
>
> #if BOOST_WORKAROUND(BOOST_MSVC, <= ....)
> # pragma warning(push)
> # pragma warning(disable:4251 4231 4660)         // Dependencies not
> exported.
> #endif

Good. I guess I should use TESTED_AT here.

> 4. the
>
> #if (defined _MSC_VER) && (_MSC_VER >= 1200)
> #  pragma once
> #endif
>
> should be added everywhere.
> I do not see it in sources I have here, maybe its old version.

It was a relatively recent addition. It seems to be on the web; maybe I didn't
update the zips. Anyway, the version you have should be almost identical.

( I actually used

  #if defined(_MSC_VER) && (_MSC_VER >= 1020)
  # pragma once
  #endif

which I copied from somewhere; this is a cruel joke because I doubt the library
will ever work for _MSC_VER < 1300.)

> 5. io/io_traits.hpp:
>
> #define BOOST_SELECT_BY_SIZE_MAX_CASE 9
>
> ==>
>
> #ifndef BOOST_SELECT_BY_SIZE_MAX_CASE
> #  define BOOST_SELECT_BY_SIZE_MAX_CASE 9
> #endif

The docs for select-by-size are here: http://tinyurl.com/3vjwf. Maybe I should
add it to the current lib docs.

The usage is supposed to be

  #define BOOST_SELECT_BY_SIZE_MAX_CASE xxx
  #include <boost/utility/select_by_size.hpp>

Including the header undef's BOOST_SELECT_BY_SIZE_MAX_CASE.

> 6. docs "Function Template close": the link to the header is broken.

Thanks. When I wrote that page operations was still in boost::detail.

> In this page: somesting like UML's sequnce or collaboration
> diagram could be added.

Good idea. Closable is actually the hardest concept to document.

> (I am visual type so I always ask for pictures and code examples.)
>
> 7. windows_posix_config.hpp: I find the macros
>    BOOST_WINDOWS/BOOST_POSIX too general for iostreams.
>    Either they should be in Boost.Config or something as
>    BOOST_IO_WINDOWS should be used.

That's one of the other changes I made late. I had been borrowing
BOOST_WINDOWS/BOOST_POSIX from Boost.Filesystem, but didn't realize until
recently that cygwin users are supposed to be able to pick either configuration.
That defintely doesn't work for my library.

>   It may also fail on exotics as AS400.

Do you mean neither configuration will work in this case? I have to figure out
graceful ways for mmap and file descriptors to fail on unsupported systems.

> 8. Maybe the library files could be more structured.
>     Right now its directories contain 32, 24, 11 and 9 files
>     and it may be confusing to orient in it.
>
>    There may be subdirectories as  utils/, filters/,
>     filters/compression/ etc.

I'm leaning in that direction. If the library is accepted, I may ask your advice
on organization after I decide what other filters to include.

> 9. Maybe assert() could be replaced with BOOST_ASSERT().

Okay. I always forget about BOOST_ASSERT. There doesn't even seem to be a link
to it from the libraries page or from Boost.Utility.

> 10. disable_warnings.hpp: maybe this and other similar files could
>       be merged into iostream_config.hpp.

I like to disable warnings at the beginning of a file, and enable them again at
the end. I only use this in a few places.

>      (Others: enable_stream.hpp.)

Okay.

> 11.  details/assert_convertible.hpp:
>
>        The macro here doesn't feel as useful too much.
>        Straight BOOST_STATIC_ASSERT(is_convertible....)
>        would take the same space and would convey more information
>        and immediatelly.

I'll probably use BOOST_MPL_ASSERT(is_convertible<.. >), now that it's
available.

> 12. detail/access_control.hpp: this feel as as it could be in boost/utility/
>       or in boost/detail/.
>
>      An example(s) could be provided in the header.

I'm glad to hear you think it may be useful.

>
> 13. detail/buffer.hpp: should have
>       #include <boost/noncopyable.hpp>
>
>
>      Commenting nitpick: maybe instead of
>
> // Template name: buffer
> // Description: Character buffer.
> // Template paramters:
> //     Ch - The character type.
> //     Alloc - The Allocator type.
> //
> template< typename Ch,
>           typename Alloc = std::allocator<Ch> >
> class basic_buffer : private noncopyable {
>
>
> could be
>
> // what it is used for....
> template<  typename Ch,
>           typename Alloc = std::allocator<Ch> >
> class basic_buffer : private noncopyable {
>
> Too many comments here are obvious and this makes
> reader easy to skip them all.

Agreed. I have a semi-standard way of documenting templates, which sometime
results in the sort of uninformative comments you quote above. But I don't
follow this pattern consistently, so there's not much point.

> OTOH it should be explained why there's basic_buffer
> and buffer and why its functionality isn't merged into one
> coherent class. This is far from obvious.

Okay. For the record:

- The extra pointers aren't needed most of the time, so basic_buffer is used for
(pretty trivial) space-saving and to emphasize that only the limited interface
is used.

> E.g. the design allows swap(basic_buffer, buffer).

I guess I could disable that.

> It should be explained why std::vector or boost::array
> isn't enough.

- vector<Ch> initializes each character
- boost::array is statically sized

> Maybe these class(es) could be factored out into boost/details/
> or standalone mini-library.
>
> 14. detail/config.hpp: the trick with
>
> #ifndef BOOST_IO_NO_SCOPE_GUARD
>
> I have feeling it should be removed. Compilers who cannot
> handle even scope guard are not worth to support.
> Or the scope_guard for these could be dummy.

Unfortunately, my scopeguard fails to work on one pretty good compiler. I assume
I'll either fix it or use Joaquín's version. Either way, these conditional
sections will be removed. But I need to be able to run the regression tests in
the mean time.

> Number of macros in iostreams library would be
> better reduced.
>
> The BOOST_IO_NO_FULL_SMART_ADAPTER_SUPPORT
> macro: it should be explained in code comment what it means.
> Maybe something like it could be moved into Boost.Config
> or maybe something like this is already in Boost.Config.

This is for Borland, which seems to go into an infinte loop of template
instantiations without this workaround. I'm not sure what the underlying problem
is.

> BOOST_IO_DECL: this should be in Boost.Config
> as BOOST_DECLSPEC. Other libraries (regex) have
> their own macros and it is all one big mess.

But it uses BOOST_IO_DYN_LINK. Are you saying users shouldn't be able to link
dynamically to selected boost libraries? Maybe it could be BOOST_DECLSPEC(IO).

> 15. converting_chain.hpp: how brackets are positioned
>      would be better unified. Here I see the
>     if (xxxxxxxxx)
>          {
>          ....
>          }

I don't see that in converting_chain.hpp. I typically write if's that way only
if the condition takes multiple lines.

>    and elsewhere Kernigham notation
>    and it makes me wonder whether whether it has some
>    hidden meaning.

Are you perhaps talking about the distintion between

     void f()
     {

     }

and

     void f()
         {

         }

?

I prefer the latter when defining functions within a class body, because it
seems more readable. But I admit I am somewhat inconsistent.

>
>    Comments in code would be welocomed here.
>

Agreed. converting_chain, converting_streambuf and converting_stream are not yet
up and running.

> 16. double_object.hpp:
>
>        typo in source "simalr"

Thanks.

>        Wouldn't it make sense to have this in compressed_pair library?

I find it very useful, but I wasn't sure if others would. Adding it to
compressed_pair makes sense. Maybe I'll start by putting it in detail.

> 17. forwarding.hpp: the macro
>       BOOST_IO_DEFINE_FORWARDING_FUNCTIONS
>       is used on exactly one place. Maybe it could be defined/used/undefined
>       here to make overall structure simpler.

That's probably a good idea. Originally it was part of detail/push, which could
be useful to end-users (when I document it). I guess when I factored out the
forwarding part I didn't realize that it had limited utility.

> 18. io/detail/streambufs/ : there are temporary files
>       indirect_streambuf.hpp.bak.hpp and indirect_streambuf.~hpp.

These are gone now.

> 19. details/chain.hpp:
>
>     #if defined(BOOST_MSVC) && _MSC_VER == 1300
>         virtual ~chain_base() { } // If omitted, some tests fail on VC7.0.
> Why?
>     #endif
>
> doesn't this change semantics of the class?

Theoretically. But it's never supposed to be used for run-time polymorphism. And
it's an implementation detail.

> Ans ASCII class diagram could be here.

I guess that't a reasonable request, since chain is the heart of the filtering
implementation.

> 20. detail/iterator_traits.hpp: looking on specializations:
>       would it make sense to have unsigned char/signed char
>       versions as well?

I guess it wouldn't hurt. But is char_traits typically specialized for these
types?

>     (There are more places with explicit instantions that would
>      need possible update).

I don't see any. Unless you mean std::char_traits ;-)

> 21. test lzo.cpp refers to non-existing boost/io/lzo.hpp.

Right. I got rid of it because of copyright issues.

> 22. io/file.hpp: what is exactly reason to have pimpl in
>       basic_file_resource class?
>
>       I do not see headers that don't need to be included,
>       I do not see dynamic swicthing of pimpls, I do not see
>       eager or lazy optimizations. I see only overhead and complexity.

Exception safety. I left this out of the latest rationale, but it's an important
part I'm going to put back in. Several iteration ago, the usage would have been:

     filtering_istream in;
     in.push(new gzip_decompressor());
     in.push(new file_source("hello.gz"));

Generalizing this convention of passing by pointer and transfering ownership led
to exception safety problems in other parts of the library. So the current
convention is that filters and resources are passed by value, and so must be
copy constructible. (streams and stream buffers are stored by reference by
default, and the same effect can be achieved for an arbitrary component using
boost::ref())

So, to answer your question: basic_file_resource wraps a basic_filebuf, which is
generally non-copyable, so I used a shared_ptr.

> 23. io/memmap_file.hpp:  why is pimpl in mapped_file_resource class?

To avoid having to include operating system headers from a header file.

> 24. io/regex_filter.hpp, function do_filter() contains:
>
>     void do_filter(const vector_type& src, vector_type& dest)
>         {
>             ......
>             iterator first(&src[0], &src[0] + src.size(), re_, flags_);
>
>      Is the &src[0] safe if vec is empty? I don't know if standard
>      allows it, it just caught my eyes.

Good point. I'd don't know if it's safe. But it's easy enough to handle this
case separately.

Thanks for the very detailed criticism!

>
> /Pavel
>

Jonathan