Hi. I checked out the optional library and found it to be of great value. I have, however, a few small suggested improvements (and no, I'm not talking about the pointer/container/discriminated-union semantics debate; I read this discussion in the mail archives and I also saw the resulted library and I'm very happy with what came of it). I'm talking about the detail::none_t part. Should it really be in the detail namespace? Isn't that a little weird? The detail namespace should be "forbidden area" for the users, yet here they are offered to write code like my_optional = boost::detail::none_t(); One might argue that there's the 'none' global variable which will make the code look like my_optional = boost::none; which looks much better. I agree that it looks much better, but it still doesn't make the previous way of thinking (crating a temporary none_t instance) illegal or unsupported. It is unsupported now. And while we're at the global none variable, I have to say that the reason I first tried the temporary-none_t-instance way, is that I didn't know of 'none'. The documentation is very poor on this subject. It's only mentioned in one place in a short, unexplained sentence, and it's in the "Detailed Semantics" section which no one ever bothers to read anyway. In addition to improving the docs, I'd say that it's even better to include the boost/none.hpp header with optional.hpp so it could be easily found and reached. As I understood, the reason why this is not done now, is because some compilers (such as Borland's) have troubles with initialized data and pre-compiled headers. If this is truely the case, then I think it's easily solved. Include as follows: #ifndef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif while the BOOST_OPTIONAL_DISCLUDE_NONE flag can be set either by the user, or automatically if a troublesome compiler is detected. If Boost.Config could automatically detect and set a BOOST_NO_INITIALIZED_DATA flag, then the optional library is left with fairly simple code. To summarize, I suggest: 1. Lift none_t from detail to boost. 2. Improve docs wrt 'none' global instance. 3. Include none.hpp with optional unless the user/compiler dislikes it. Makes any sense? Yuval
Yuval Ronen wrote:
Hi.
I'm talking about the detail::none_t part.
Should it really be in the detail namespace? Isn't that a little weird? The detail namespace should be "forbidden area" for the users, yet here they are offered to write code like
my_optional = boost::detail::none_t();
I agree that this is an incorrect use of boost::detail.
In addition to improving the docs, I'd say that it's even better to include the boost/none.hpp header with optional.hpp so it could be easily found and reached. As I understood, the reason why this is not done now, is because some compilers (such as Borland's) have troubles with initialized data and pre-compiled headers. If this is truely the case, then I think it's easily solved. Include as follows:
#ifndef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif
while the BOOST_OPTIONAL_DISCLUDE_NONE flag can be set either by the user, or automatically if a troublesome compiler is detected. If Boost.Config could automatically detect and set a BOOST_NO_INITIALIZED_DATA flag, then the optional library is left with fairly simple code.
The problem is that people may write code expecting it to be portable and then discover that it fails to compile on some platforms. Making users include a separate header forces them to confront the problem earlier; in particular it gives them a chance to decide whether using none is worrth the cost of giving up precompiled headers on some platforms. Jonathan
Jonathan Turkanis wrote:
Yuval Ronen wrote:
In addition to improving the docs, I'd say that it's even better to include the boost/none.hpp header with optional.hpp so it could be easily found and reached. As I understood, the reason why this is not done now, is because some compilers (such as Borland's) have troubles with initialized data and pre-compiled headers. If this is truely the case, then I think it's easily solved. Include as follows:
#ifndef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif
while the BOOST_OPTIONAL_DISCLUDE_NONE flag can be set either by the user, or automatically if a troublesome compiler is detected. If Boost.Config could automatically detect and set a BOOST_NO_INITIALIZED_DATA flag, then the optional library is left with fairly simple code.
The problem is that people may write code expecting it to be portable and then discover that it fails to compile on some platforms. Making users include a separate header forces them to confront the problem earlier; in particular it gives them a chance to decide whether using none is worrth the cost of giving up precompiled headers on some platforms.
I'm not sure about that. There are plenty of places in Boost where certain features are suported only by some compilers because other compilers have problems. So it's not the only place where it's quite possible to write code that seems to be portable but is actually not. The chance to decide whether to use none or precompiled headers is still there. It didn't go anywhere, and it's not hidden or "swept under the rug" in any way. I'd even say that making these users different than others (don't get automatic none) actually raises the issue to be even more visible.
In addition to improving the docs, I'd say that it's even better to include the boost/none.hpp header with optional.hpp so it could be easily found and reached. As I understood, the reason why this is not done now, is because some compilers (such as Borland's) have troubles with initialized data and pre-compiled headers. If this is truely the case, then I think it's easily solved. Include as follows:
#ifndef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif
And I forgot BOOST_OPTIONAL_INCLUDE_NONE to force inclusion for Borland users who don't use precompiled headers, or in general users who think they know what they're doing.
"Yuval Ronen" <ronen_yuval@yahoo.com> escribió en el mensaje news:cvdm50$36d$1@sea.gmane.org...
Hi.
I checked out the optional library and found it to be of great value. I have, however, a few small suggested improvements (and no, I'm not talking about the pointer/container/discriminated-union semantics debate; I read this discussion in the mail archives and I also saw the resulted library and I'm very happy with what came of it).
I'm talking about the detail::none_t part.
Should it really be in the detail namespace? Isn't that a little weird? The detail namespace should be "forbidden area" for the users, yet here they are offered to write code like
my_optional = boost::detail::none_t();
You're definitely right about the fact that users shouldn't been doing that. At first I thought that you "cheated", like finding yourself the existence of none_t, but then I look at the documentation and is there as "detail::none_t"... This shouldn't appear in the reference!
One might argue that there's the 'none' global variable which will make the code look like
my_optional = boost::none;
which looks much better. I agree that it looks much better, but it still doesn't make the previous way of thinking (crating a temporary none_t instance) illegal or unsupported. It is unsupported now.
Right... users should be allowed to use their own none_t instance the way you did. So one clear fix is to move none_t out of the detail namespace.
And while we're at the global none variable, I have to say that the reason I first tried the temporary-none_t-instance way, is that I didn't know of 'none'. The documentation is very poor on this subject. It's only mentioned in one place in a short, unexplained sentence, and it's in the "Detailed Semantics" section which no one ever bothers to read anyway.
Right, so the next fix is to improve the documentation.
In addition to improving the docs, I'd say that it's even better to include the boost/none.hpp header with optional.hpp so it could be easily found and reached. As I understood, the reason why this is not done now, is because some compilers (such as Borland's) have troubles with initialized data and pre-compiled headers. If this is truely the case, then I think it's easily solved. Include as follows:
#ifndef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif
while the BOOST_OPTIONAL_DISCLUDE_NONE flag can be set either by the user, or automatically if a troublesome compiler is detected. If Boost.Config could automatically detect and set a BOOST_NO_INITIALIZED_DATA flag, then the optional library is left with fairly simple code.
Well, I'm not sure it is that easy. Since BOOST_OPTIONAL_DISCLUDE_NONE will be compiler specific, most code will have to add the counterpart: #ifdef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif somewhere else, otherwise none_t will be just equally undefined. So the "complete" solution seems too contrived to me given that the only real problem here is just that the documentation is incomplete and users wanting to use "none" don't have a clue about how to do it. I believe that with a clear documentation users won't have any problem including none.hpp manually in just the right place.
1. Lift none_t from detail to boost. OK
2. Improve docs wrt 'none' global instance. OK
3. Include none.hpp with optional unless the user/compiler dislikes it.
Not so sure ;) Thank you for the report! Fernando Cacciola
Fernando Cacciola wrote:
"Yuval Ronen" <ronen_yuval@yahoo.com> escribió en el mensaje
In addition to improving the docs, I'd say that it's even better to include the boost/none.hpp header with optional.hpp so it could be easily found and reached. As I understood, the reason why this is not done now, is because some compilers (such as Borland's) have troubles with initialized data and pre-compiled headers. If this is truely the case, then I think it's easily solved. Include as follows:
#ifndef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif
while the BOOST_OPTIONAL_DISCLUDE_NONE flag can be set either by the user, or automatically if a troublesome compiler is detected. If Boost.Config could automatically detect and set a BOOST_NO_INITIALIZED_DATA flag, then the optional library is left with fairly simple code.
Well, I'm not sure it is that easy. Since BOOST_OPTIONAL_DISCLUDE_NONE will be compiler specific, most code will have to add the counterpart:
#ifdef BOOST_OPTIONAL_DISCLUDE_NONE #include <boost/none.hpp> #endif
somewhere else, otherwise none_t will be just equally undefined.
Well, I guess that it depends on how portable the code should be. If the user is writing code without much of portability requirements, then he can leave it for the Boost auto-detection configuration, which means that for most users with most compilers, none.hpp will be included at no extra work. This is something I would really like to have. Users who write code which must be very portable, can include none.hpp specifically, even without the #ifdef. The worst that can happen is that this header will be included twice on some compilers. Not a big deal IMO.
So the "complete" solution seems too contrived to me given that the only real problem here is just that the documentation is incomplete and users wanting to use "none" don't have a clue about how to do it. I believe that with a clear documentation users won't have any problem including none.hpp manually in just the right place.
This feature I asked will make my life nicer, sure, but on the other hand, not as much as world peace, so I won't make that a huge fuss of it if weren't to be implemented... ;-)
1. Lift none_t from detail to boost.
OK
2. Improve docs wrt 'none' global instance.
OK
3. Include none.hpp with optional unless the user/compiler dislikes it.
Not so sure ;)
Thank you for the report!
Fernando Cacciola
Thank you very much for listening! Yuval
participants (6)
-
Fernando Cacciola -
Jonathan Turkanis -
Yuval Ronen