[boost] Re: IOStreams formal review start

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2004-09-10 05:04:27

"Jonathan Turkanis" wrote:

_______________________________________________
> > 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.
>
I mean someone may define the value
BOOST_SELECT_BY_SIZE_MAX_CASE in commandline
and it would be ignored.

_______________________________________________
> > 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.
>
I don't know. I just have some strange feeling something
could go wrong. Maybe older Macs could have problems here.

This should be solved 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.
>
> 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).
>
My opinion is BOOST_DECL should exist in Boost.Config.

_______________________________________________
> > 21. test lzo.cpp refers to non-existing boost/io/lzo.hpp.
>
> Right. I got rid of it because of copyright issues.
>
That's unfortunate. Maybe it could be there as example.

> > 22. io/file.hpp: what is exactly reason to have pimpl in
> >       basic_file_resource class?
> >
> Exception safety....
>
> So, to answer your question: basic_file_resource wraps a basic_filebuf,
> which is generally non-copyable, so I used a shared_ptr.
>
Hmmm. Would it be possible to use intrusive_ptr here?

_______________________________________________
> > 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.
>
But io/detail/memmap.hpp is included and must be included anyway.
Or do I miss something?

/Pavel