"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
- 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
I do not have clear picture: when io::open() is called the real type of filter is not known? ____________________________________________________ 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