[boost] Re: IOStreams formal review start

From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-09-10 10:44:59

"Pavel Vozenilek" <pavel_vozenilek_at_[hidden]> wrote in message
news:chru8b$ik8$1_at_sea.gmane.org...
>
> "Jonathan Turkanis" wrote:

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

I see; the docs say BOOST_SELECT_BY_SIZE_MAX_CASE should be defined right before
use. The command-line seems a bit to far removed to make sure that enough cases
are covered, and

> > > #ifndef BOOST_SELECT_BY_SIZE_MAX_CASE
> > > #  define BOOST_SELECT_BY_SIZE_MAX_CASE 9
> > > #endif

seem like unnecessary work for users.

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

In theory, I'd like to provide implementations for all boost-supported platforms
that support file-mapping and have runtime "can't map file" errors for other
platforms.

Currently, following Boost.Filesystem, BOOST_IO_POSIX is defined by a process of
elimination. On unsupported systems users will get compiler errors such as
"can't open file 'sys/mman.h'; no such file."

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

I guess this should be taken up with John Maddock.

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

Doesn't the boost license policy cover examples, too?

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

I think so. Why do you think it's important?

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

I misread your question. detail/memmap.hpp uses the pimpl idiom to avoid
including operating system headers. for boost/io/mapped_file, the reason for
shared_ptr is the same as for bost/io/file.hpp.

>
> /Pavel
>

Jonathan