Re: More on the optional class.

From: Gennadiy E. Rozental (rogeeff_at_[hidden])
Date: 2001-09-16 03:26:18

--- In boost_at_y..., "Fernando Cacciola" <fcacciola_at_g...> wrote:
> The following is derived from conversations with Gennadiy Rozental 
about the
> optional<> class.
> 
> My initial design used a mechanism that allows optional<T> to 
bypass T::T().
> This capability is required in order to allow a type without a 
default
> constructor to be used with optional<T> ; and it also offers room 
for
> optimization since the default constructor may not be trivial.
> 
> Gennadiy was concerned about the unnecessary overhead introduced by 
the
> bypass mechanism in cases where T is a POD; and he made a good job 
at
> drawing my attention to this issue.
> 
> There are two possible implementations:
> 
> a value-based implementation, where optional<T> contains:
> 
>   T v ; bool  initialized ;
> 
> and a pointer-based implementation, where optional<T> contains:
> 
>    T* v ;  aligned_storage<T> buffer;
> 
> I setup a test bed and verified that for POD types, the overhead is 
about as
> twice as much executed code for the typical operations.
> Therefore, I decided to do the following:
> 
> 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. 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.
  Lyrical 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.

That is why I think that in most cases value based bahaviour would be 
used and propose it as default case for boolean argument. In those 
rare cases when you whould need pointer based behavior you can 
specifically specify that by second template argument. But if 
everybody are disagree with my points, I admit that current way to 
customize the internal behaviour is acceptable

> 
> I change the code to reflect this.
> The new optional<T> uses ::boost::is_POD<T> to select a value or
> pointer-based implementation.
> The interface allows the user to override the default impl 
selection for any
> type T to accommodate his/her needs.
> 
> I also made up my mind about how to let the user test if a given 
optional is
> initialized or not.
> Previously, optional<> used operator void*() to allow expressions 
of the
> form:
> 
>   if ( opt ) or if ( opt != NULL )
> 
> 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 just hope enough people is aware and fond of this very useful 
technique,
> because it removes all the problems with the other approaches.
> The new optional.hpp header contains a small rational for this 
design
> decision.
> 
> I also refactored things and updated the documentation and the test 
bed.
> The new stuff has been updated in the FilesSection under optional 
class (as
> before).
> I also included a zip file for easier downloading.

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 ;-)))

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?)

line 166:
    optional () // no throw

Some comments ( including big description in the beggining) reflect 
pointer-based behaviour, which is not the case any more.

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?

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?

line 226:
    operator !() const { return !initialized() ; }

Wouldn't it be better to put bool ahead?

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.

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.

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.

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

line 80
     void __export accessing_uninitialized_optional_error() ;

Does __export is standart keyword?

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;
 

line 124-125:
         T v ;
         bool init ;

Why do you have them in public section of class?

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).

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.

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).

> 
> Opinions are welcome.
> Thanks,
> 
> Fernando Cacciola
> Sierra s.r.l.
> fcacciola_at_g...
> www.gosierra.com

This is my opinions.

Regards,

Gennadiy.