Douglas Gregor said: > The formal review of Fernando Cacciola's Optional library begins today > and runs until the end of Wednesday, December 18. > > The Optional library provides a class template "optional<T>" that either > contains a value of type "T" or contains no value (i.e., having a value > is optional). It is useful, for example, as the return type of > functions that may or may not return a value, depending on their > inputs. > > The Optional library can be downloaded from the Boost files section: > http://groups.yahoo.com/group/boost/files/optional.zip > > Please state in review comments whether or not you believe that the > library should be accepted into Boost. Further guidelines for writing > reviews can be found here: > http://www.boost.org/more/formal_review_process.htm#Comments
I vote to accept. I see many uses for this concept, several of which aren't even dealt with in the documentation. Specific comments: * I believe there should be an "optional& operator=(T const& v)". * I think "peek()" should be named "get()". Despite it's unusual (and needed) use, this is a form of a smart pointer, and should follow the same naming conventions that people have grown accustomed to through std::auto_ptr and the Boost smart pointers. * I'm unsure about the need for "value()". I'd be happy with just the normal smart pointer interfaces. * I think "uninitalize()" should be named "reset()". Not only for consistency with other smart pointers, but also because the term isn't really even meaningful english. * I'm unsure about the presence of "initialized()". On the one hand, the duplication in features (compared to "get/peek() == 0") is something I think designs should generally avoid. On the other hand, this name is more meaningful for what precisely "get/peek() == 0" signifies. I guess I'm +0 on this one. * I'm a little uncomfortable with the free functions in <boost/optional.hpp>. With out some rationale for them they seem to provide nothing and are useless duplications in the public interface, which will lead to confusion for users. * I don't follow the rationale for not having relational comparisons. When would you ever get a "maybe" state when comparing? I can see that when comparing two uninitialized optionals that a case could be made for either true or false, but that's not a "maybe" as to my mind you could pick one of them with out causing any ill effects to anyone, and if you do it seems clear to me that you'd want to choose true. I'm not (necessarily) arguing for the comparisons, I'm just questioning the rationale, at least as it's worded in the document now. * I also don't follow the rationale for leaving out the conversion to bool. I see no conflict with including this conversion, and in fact inclusion would make this more consistent with other smart pointers. There's no confusion/conflict created for boost::smart_ptr<bool>, so I don't see how there could be for optional<bool> either. * The fact that optional<T> does not require T to have a default constructor is significant, and should have some more emphasis in the documentation. This is what opens up use cases other than the "return a value or uninitialized" case discussed ad nauseum in the document. For instance, I expect to be able to put this class to use in an implementation of an "async_result" type for Boost.Threads. All of these comments are based on the design/interface and not the implementation. I'll look at the implementation when time permits, but don't expect anything to change my vote for inclusion. William E. Kempf _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
