Re: [boost] Formal review: Optional library

From: William E. Kempf (wekempf_at_[hidden])
Date: 2002-12-09 13:53:45

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