RE: [boost] Re: More on the optional class.

From: Fernando Cacciola (fcacciola_at_[hidden])
Date: 2001-09-17 13:34:57

----- Original Message -----
From: Gennadiy E. Rozental <rogeeff_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Sunday, September 16, 2001 5:26 AM
Subject: [boost] Re: More on the optional class.

> [SNIP]
>
> > I believe that the implementation choice for optional<T> depends
> *only* on
> > T -not in the context in which optional<T> is used-; therefore, the
> optional
> > class can automatically select the appropriate implementation based
> only on
> > the type T. No policy class is required, or at least, not as a
> second
> > template argument.
>
> I agree with premise, but I do not completely agree with conclusion,
> specifically with the provided mechanism for optional internal
> behaviour customization. Though I agree that Polycy template argument
> seems to be unnecessary overhead, specializing optional_behaviour to
> enforce value based implementation also  also seems to be too
> complex, especially if I would need it to do for every type that I am
> using with optional.
If you were to specify the behaviour for every type then you would be right,
but AFAICT, you would need to do it only ocasionally. Optional offers a
default selection which I believe provides a reasonable choice.

> The simple solution could be just to provide
> boolean template argument as selector. More complex solution is to
> provide type argument that could be true_type, false_type or default -
> where third case would mean current selection logic.
But this still has the main drawback of a policy class: a second template
parameter.

Let me explain: Classes optional<T,A> and optional<T,B> are different
classes. However, they are -or should be- *conceptually* the same, since the
template parameters A and B are implementation details, not a customizable
part of the interface.
I strongly believe that additional template parameters are not the proper
way to provide implementation customization, precisely because they
introduce different classes.
How would a user write a function that takes an "optional int"? void foo
 optional<int,true> const& opt) ?
What if I defined optional<int,false>? What if some other user defined
optional<int,true>? Does it means that there are two 'optional int' classes?
If they behaved differently I would said yes, but the differ only in
implementation, so I don't think both should exist.

I said that the implementation choice for optional<T> depends only on T. I
meant that it is required -by my design- that only a *single* optional class
can exist for each type T.  This cannot be enforced if I include any extra
template parameter.

Perhaps my solution was not the best, and I will be glad to see anything
better, but we must be careful about all the impacts of any design decision.
As I said, I think that a second template parameter is just not an option.

> Logical digression:
>   I still think that usage of pointer-based optional seems to be a
> little bit odd. Given bulky or expensive to construct type T I would
> rather use plain pointers or to simplify memory management -
> shared_ptr. In your example:
>
> optional<T> array[N];        vs     shared_ptr<T> array[N];
>
> 1. Does not call constructor        1. Does not call constructor
>  at initialization stage             at initialization stage
> 2. Does not call destructor for     2. Does not call destructor for
>  elements that did not constructed   elements that did not constructed
> 3. Value acess overhead the same    3. Value acess overhead the same
>  as with plain pointer               as with plain pointer
> 4. "Allocate" sizeof(T)*N memory    4. Memory allocation strategy is
>  on stack. May not use all of it.    configurable by allocator
>  In case of bulky type T it          parameter. May behave the same
>  could be a big memory overhead.     way as optional<T> with 1 time
>  This could be especially            memory allocation in a heap which
>  important in system with small      add neglectable overhead.
>  memory                              Alternatively it could use some
>                                      kind of smart strategy to manage
>                                      memory usage.
>
I don't think you are being fair in this table, you are underestimating the
complexity of any allocation strategy required to deal with true pointers.
For instance, you mention an 'allocator parameter'; if you referred to the
vector's allocator, you still need to allocate by your self every T object
before you wrap it in a shared_ptr<>.
You also mentioned that you can achieve 'the same 1 time memory
allocation...'; I don't think that's true, simply because my example
presents 0 (zero) heap allocations. Anyway, suppose I did 1 heap allocation,
can you show me *exactly* how would you reproduce that using shared_ptr<>?
(remember that even if you use a pool you still cannot guarantee a single
allocation, except in a very fixed situation).

> [SNIP]
> > After *a lot*  of consideration, I decided to support only
> the 'bang-bang'
> > idiom:
> >
> >   if ( !!opt ) means initialized.
> >   if ( !opt) means uninitialized.
>
> I admit that this is good solution if you want to eliminate any kind
> of implicit conversion from optional to T
>
I also like Peter's suggestion:  if ( T* p = get(opt) ) ...

> [SNIP]
> Now about implementation (I am not familiar with aligned storage side
> so you won't see any comment on that):
>
> optional.hpp:
>
> line 17:
> #include "boost\type_traits.hpp"
>
> I would recomend to include specific traits header. No need to make
> compiler life even more complex ( be pitifull to them. They are
> trying so hard to be up to standart and work properly with all this
> template madness ;-)))
>
Well, if this is the proper way to do that I will change it, however, the
documentation mentions only 'type_traits', so I'm not sure if it is OK to
include 'object_traits.hpp" directly.
Jhon?

> line 155-160;
> It just my general preferance to see first public interface then
> followed by private implementation. It seems to be more reasonable,
> though it is up to you (does boost have an appropriat guidance?)
>
I would agree, but my compiler (BCB5) can't handle these declarations at the
bottom, so I had to move them up.

> line 166:
>     optional () // no throw
>
> Some comments ( including big description in the beggining) reflect
> pointer-based behaviour, which is not the case any more.
>
Can you elaborate this, please? I don't follow you

> lne 190-191:
>     T const* get() const { return imp.get() ; }
>     T*       get()       { return imp.get() ; }
>
> Frankly speaking, I don't get it. After all those discusstion about
> preventing user from making occasional error at least by means of
> exception, you now allow to write:
>
> optional<T> t = foo();
>
> T t1 = *t.get();
>
> and wouldn't hit him on hands? While for pointer-based behaviour it
> will just cause memory access violation, for value-based behavoiur it
> will silently gives an access to uninitialized value. Why not put
> verify_is_initialized() check in get functions either?
>
Good catch!
My original value-based optional (which was never posted), returned NULL if
optional wasn't initialized. I just lost this when I factored out the two
implementations.
The value_based_optional<T>:get() MUST be:

         T const* get() const { return init ? &v : NULL ; }
         T*       get()       { return init ? &v : NULL ; }

this way, get() behaviour is consistent and safe.

> line 222: also see the implementation in the optional_detail.hpp
>     proxy    operator *()       { return proxy(this) ; }
>
> I think it would be better to use reference in implementing of proxy
> class. The idea id to make assignent as transparent as possible. So
> statememt like:
>
> optional<T> t;
>
> *t = some_value;
>
> would be interpreted by the compiler the same way as ( in value=based
> behavoiur)
> t.imp.v = some_value;
>
> I think with reference we have more chances.
>
>   Lyrical digression:
>  I think the rational should be to use use C++ pointer when you
> need "Pointer" that can be uninitialized (point nowhere) or should be
> able to change object it is pointed to. In other cases better to use
> C++ reference instead. What do you think?
>
Certainly. I'm not sure if the compiler will actually generate any better
code with a reference, but a pointer is not *required* so you are right.

> line 226:
>     operator !() const { return !initialized() ; }
>
> Wouldn't it be better to put bool ahead?
>
Absolutely!!

> line 228:
> static std::string implementation_choice()
>
> I am not sure why do you need that function (other than for internal
> testing). This couse you to include <string> header(and <typeinfo>
> which is not present now but will be required by other compilers),
> which is not used anywhere else.
>
This is for internal testing only.
I figured that a user might need to know which implementation is using for a
given T, because otherwise he/she wouldn't know if he/she should override
the default selection.
I think I will remove this, because as you properly pointed out, this
requires two otherwise unnecessary headers.

> line 241:
>       #ifndef _NDEBUG
>
> I am not sure about this name. Should we use some specific name
> instead (like DEBUG_OPTIONAL)?
>
> line 243: also see the implementation in the optional_detail.hpp
>      optional_detail::accessing_uninitialized_optional_error() ;
>
> Well, it is another general concern about design. I think that in
> most cases you just need assert based implementation for this
> function. The idea was to provide developer an information: Hey you
> forgot "is initialized" check. Developer type it in, compile it and
> move on. This error is not supposed to be cought somewhere, reported
> and than - keep working. If you so desperately need exception based
> error check provide a way to cusomize it. But default still sould be
> assert as more straight way.
>
I'm intentionally not taking much care about this precondition issue. There
are a lot of things to account for that require a more group-like
discussion. I expect that this class be eventually formally reviwed, and
then I will like to see what are the general alternatives in this respect. I
think some sort of 'general boost approach' might be the only complete
solution, and I'm not refering to simply resort to 'assert', as I explained
in a previous message.
(For example, my own class is totally different in this respect, because I
have my own policies, switches and helper classes; but I can't use them in
the posted code).

>
> optional_detail.hpp
>
>
> line 19:
> #include "boost\type_traits.hpp"
>
> see above
>
> line 17,20:
> #include<new>
> #include "boost\aligned_storage.hpp"
>
> It would be great to separate to behavoiurs so that value-based one
> won't include those both header. Again - no need to enlage work for
> compiler. But I still do not now easy way to do this.
>
optional.hpp requires both, so I don't think there is any gain in that.

> line 36:
>          OPT* x ;
>
> see above
>
>
> line 48-76
> Comparison operators.
>
> I still does not convinced should we or not provide templated
> versions to allow implicit conversion so that
>
> *t = u will compile without explicit cast (*t = (T)u;). I would like
> to hear some other opinions on this metter. You can find our
> discussion in http://groups.yahoo.com/group/boost/message/17074 and
> http://groups.yahoo.com/group/boost/message/17397
>
I would also like to hear other opinions about this.

> line 80
>      void __export accessing_uninitialized_optional_error() ;
>
> Does __export is standart keyword?
>
No it isn't. But this function should be reachable from any user code. In
the platforms I know of, some sort of export keyword is required to achieve
that.
My intention by leaving the keyword there was to emphatize that. As with the
rest of the checking code, I expect to discuss this with the rest of the
group. The same goes for the existence of the 'optional.cpp' file.

> line 106, 112:
>              v = av ;
>
> This statement could cause en exception. WE should not leave the
> object in undefined state The flag  should be reset to false. It
> could be done be means of try-catch block where in catch we set init
> to false and then rethrow or ,simplier, just adding additional
> statement ahead of this init = false. So the assign will look like
> this:
> init = false;
> v    = av;
> init = true;
>
There's no doubt about the fact that the object MUST be left uninitialized
in case of an exception.
The optional design *requires* a type T for which a value-based
implementation is used to be a POD type, which requires it not to throw in
any of T::T(), T::T(T const&), T::operator =(T const&) or T::~T().
This is why, unlike the pointer-based implementation, this implementation
doesn't provide any exception safety.
However, it might be possible that the user defined a value-based behavior
for a non-POD type, so your simple guard of setting init to false before the
assignment seems fine, and fast enough to be used even while normally it
isn't necessary. I'll add that.

> meta_tools.hpp
>
>
> line 32:
>     template<int N> struct typed_int_value
>       { BOOST_STATIC_CONSTANT(int, value = N ) ; } ;
>
>   Lyrical digression:
>   This is an idea I was thinking to implement for some time now.
> Solaris SunPro does not support any expression with non-type template
> arguments and this is one of my target compilers. But you missing
> very important thing: prev and next (you will need preprocessor to
> define enough specializations ):
>     struct begin{};
>     struct end{};
>     template<int N> struct typed_int_value
>       { BOOST_STATIC_CONSTANT(int, value = N ) ; } ;
>
>     template<> struct typed_int_value<0>
>

>          BOOST_STATIC_CONSTANT(int, value = 0 );
>          typedef begin prev;
>          typedef typed_int_value<1> next;
>
>       } ;
>
> ....
>
>     template<> struct typed_int_value<MAX_NUM_TYPE>
>

>          BOOST_STATIC_CONSTANT(int, value = MAX_NUM_TYPE );
>          typedef typed_int_value<MAX_NUM_TYPE-1> prev;
>          typedef end next
>
>       };
>
> Now you will be able to use this facility to implement some of the
> compile-time tricks you would like to use (like GCD library currently
> under review with some additional efforts).
>
This is fine. I've seen this already somewhere in the boost headers (in mpl
maybe).

>
> line 50-52:
> template<class Cond,class T0,class T1> struct select ;
> template<class T0,class T1> struct select<type_true,T0,T1>
> { typedef T0 type ; } ;
> template<class T0,class T1> struct select<type_false,T0,T1>
> { typedef T1 type ; } ;
>
> There are couple versions of Meta IF that does not use partial
> specialization. Look for example on changes proposed by me for select
> type header file in http://groups.yahoo.com/group/boost/message/17206.
>
I'll look at it. Though, I'm not sure if the other techniques I've seen are
actually better -in terms of compiler support-

> line 56 -63:
>     struct end {};
>
>     template<typename Head, typename Tail = end>
>     struct list
>     {
>       typedef Head head_type;
>       typedef Tail tail_type;
>     } ;
>
> This is basically loki's TypeList. So the name should be typelist and
> implementation could be much more powerfull (plus we could use
> typed_int_value to make it more portable).
>
This is Douglas Gregor's code. I've just borrowed it and I'm not going to
change it without his concent.

Anyway, regarding meta_tools.hpp:

I wasn't really trying to provide a generally useful meta programming
toolset. The mpl, and other stuff already on boost, are far better designed
than this, but some of then use non-type template parameters or simply crash
BCB5 (like mpl, for instance), so I did it myself, but without generality in
mind.
I created a separate header with the hope of eventually replacing it
entirely with something more well thought (such as mpl, for instance).

> >
> > Opinions are welcome.
> > Thanks,
> >
>
> This is my opinions.
>
> Regards,
>
> Gennadiy.
>
Thank you for such a deep look!

Best regards,

Fernando Cacciola
Sierra s.r.l.
fcacciola_at_[hidden]
www.gosierra.com