[boost] Re: IOStreams formal review start

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2004-09-09 12:27:39

"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 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?

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

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

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

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

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

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

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.

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

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

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

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

  It may also fail on exotics as AS400.

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.

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

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

     (Others: enable_stream.hpp.)

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.

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.

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.

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.

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

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

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.

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.

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.

15. converting_chain.hpp: how brackets are positioned
     would be better unified. Here I see the
    if (xxxxxxxxx)
         {
         ....
         }
   and elsewhere Kernigham notation
   and it makes me wonder whether whether it has some
   hidden meaning.

   Comments in code would be welocomed here.

16. double_object.hpp:

       typo in source "simalr"

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

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.

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

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?

Ans ASCII class diagram could be here.

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

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

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

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.

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

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.

/Pavel