Re: [boost] Formal review: Optional library

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2002-12-09 15:43:13

"William E. Kempf" <wekempf_at_[hidden]> wrote in message
news:1771.167.16.75.11.1039460025.squirrel_at_frodo.kempf-ville.com...
>
> 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.
>
That's great!

> I see many uses for this concept, several of which
> aren't even dealt with in the documentation.
>
OK, I'll see to add them to the docs.

>Specific comments:
>
> * I believe there should be an "optional& operator=(T const& v)".
>
This would break the pointer-semantics:

optional<int> opt ;

opt = 3 ;

instead of

*opt = 3 ;

as it is currently the required syntax.

> * 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.
>
Fair enough.
I'm actually trying to vouch for my peek/acquire idiom here.
Essentially, the name 'peek()' is intended to convey the fact that the
pointer
points to a managed object and that the ownership is not being transfered
along with the pointer (as oposed to acquire()).
If I manage to make the idiom known enough, the user will know that he
can't delete the pointer and that the pointer can be used only
as long as the 'source' (the optional<> object in this case) remains alive.

> * I'm unsure about the need for "value()".  I'd be happy with just the
> normal smart pointer interfaces.
>
value() is there just for those contexts in which the conversion
from the proxy (returned by operator*()) to 'operator T&' is not pick up by
the compiler.
For example, when you pass (*opt) to a template function.
You could use: *peek(opt), but peek() doesn't test for the initialize state
first,
as does value().

> * 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.
Hmmm.
Does 'reset' conveys what it is doing?
Is it clear that 'reseting' an optional<> means to leave it uninitialized?
I'm not sure; but if others see it like you do, I'll change it.

>
> * 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.
>
To be honest, I dislike it too :-)
But some people found the alternative spellings ugly,
so I figured that a member function would make them happy.

> * 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 generally dislike member functions for 'general' objects.
Onn several contexts, the optional<> objects can be replaced by true
pointers directly
thanks to its pointer semantics (and correspoding syntax)
With the non-member interface, you can write code that works even if you
change optional<>
with a true pointer.
The member function interface is not-recommended.
It is there only for those situations in which resolution problems prevent
the user
from using the non-member functions.

> * I don't follow the rationale for not having relational comparisons.
> When would you ever get a "maybe" state when comparing?
Yes, whenever one of the optional<> is uninitialized.

>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.
>
optional<> is intended to be used as much as a pointer as possible;
therefore
the syntaxis of its usage is intended to looks like that of a pointer;
i.e., you can't access optional's "value" directly,
you need operator*() for that.

Consider:

int* a;
int* b;
if ( a == b ) ..

Altough the comparison is well defined between pointers,
you are not comparing the "values of the objects" being pointed to,
only the pointer values.
If you want to compare object values, you must dereference the pointers:
if ( *a == *b ) ..

optional<> follows the same idiom.
You need to use the operator*() to perform the comparisons because
you need to dereference an optional<> in order to access its value.
For this reason, the proxy, returned by operator*(),
does have some conditional operators (== and !=) defined,
so you can compare optional "values".

> * 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 conversion to bool for a smart pointer unambiguously refers to the
NULL state of the pointer wrapped.
In the case of optional, a conversion to bool will be ambiguous in those
cases were the wrapped T in optional<> is bool itself.

> * 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.
I see.
Could you give me a summary of the usage in async_result?
I can add it as the list of usages.
I know we have talked about this but I don't remember the details.

>
> 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
>
Thanks!

Fernando Cacciola