[boost] Re: Formal Review: Variant Library

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2003-02-18 15:55:39

"Eric Friedman" <ebf_at_[hidden]> wrote in message
news:b2s4b1$eof$1_at_main.gmane.org...
> Fernando Cacciola wrote:
> > I'm trying the variant library, by currently it doesn't
> > compile with bcc551.
> > I'd like to be able to compile it with my compiler so I can base my
> > vote (tentatively to accept it) on more than the documentation
> > and interpretation of the code.
>
> Thanks for the tentative acceptance. Is there anything so far in the
> documentation or your interpretation of the implementation on which you'd
> like to comment?
>
Yes, but all just trivial issues... see below

>
> > The first problem is that the current
type_traits/type_with_alignment.hpp
> > does not expose max_align any more for the Borland compilers.
> > The second problem is the extensive usage of non-type template
parameters
> > that don't follow the guidelines for integral constant expressions
> > (full qualified access). For instance, as a rule of thumb, don't use:
> > "bool_c<xyz::value>" but "bool<xyz>"; both mpl and the new type traits
> > have been designed to work with the second form which works with
Borland.
>
> Where is bool<>? (And how would that even work, seeing as bool is a
reserved
> keyword?)
>
> I don't see it in the Boost 1.29 release or the main CVS. Where are you
> looking? Or exactly what technique are you describing?
>
Sorry... I typed too fast.
I'll describe below the sort of required changes.

> > Since the bcc5.5.1 command line compiler is available from
> > http://www.borland.com/index.html
> > I ask the variant developers to download it and make the necessary
> changes.
> > AFACIT there are many changes but they're all trivial.
> > I can assist them by indicating what to change, but I don't have the
time
> > right now
> > to made the changes myself.
>
> Getting variant to work under just two compilers (GCC 3.2 and MSVC7) as
> required by the formal review submission guidelines has been sufficiently
> time-consuming. This is why more compilers are not already supported.
>
I see.

> However, I *am interested* in porting variant to other compilers in the
> future, and I do welcome any assistance in this regard.
>
All right.
My initial requirement to be able to compile the library in order to vote
was biased by the complexity of the implementation (I wanted to
make sure it worked as intended).
However, after staring at it more closely I can now follow the
implementation
and see for myself that it is good enough.

After looking closely at the documentation, implementation and examples
I vote to ACCEPT the library. Comments follow:

** documentation **

a) In the introduction appears the term 'inhomogeneous'; isn't the term
'heterogeneous' a much more familiar synonym?

b) The first variant program in the tutorial is actually full of errors :-)

c) The docs says that at least two parameters should be supplied.
This isn't true.
In fact, the implementation itself uses variants of a single parameter.

d) The docs don't mention at all the possibility of using an mpl
seuqence as the type set. Even if this is a restricted feature
-for those good compilers-, it should be mentioned because it's
most useful.

e) Th visitation examples mismatches 'print_int_float_visitor'
with 'print_int_char_visitor'

f) In the Visitor concept docs, it says:
   ...a function object which unambiguously accepts any value of
   each of the variant's bounded types...

First, I think this description is too informal, it itsn't clear if the
visitor must have overloads for all of the types _separately_,
or if the arguments must be passed by (const) reference, etc...
Perhaps something like:

   ...a function object whith an overloaded set of function call operators
   such that any value of each and all of the variant's bounded types
   can be passed to one of the operators unambiguosly, probably
   requireing an implicit conversion.

** design **

I've found the design overally satisfactory.
The overload-resolution based value-semantics are a winner IMO.
I'm not so sure about the usability of the incomplete-type support
given its heap usage, but that's a corner feature and I can't see
any better way.
I ditto others comments about support for 'void' as one of the types
with the corresponding change in 'empty()' semantics.
I also like the 'access' (currently extract) version that returns T*
with NULL meaning that the currenty type isn't T. This would leverage
handy idioms.

** implementation **

First of all, I'd like to congratulate the authors for a very clear
implementation!
I've been able to follow it even though it is rather complex and
heavy on metaprogramming. Good work!

a) static_visitable seems to be needed/useful for visitable-types, yet
it is undocumented.

b) I tend to disklike librariers with warnings about unreacheable code
 and fixed conditions.
The main conditional in 'using_storage1' and related code will have this
if there are no throwing types in the set.
At least for Borland, it would be better to enclose the code between:

c) 'which' and realated is inconsistently typed:
     the data membe is of type 'int',
     while the return type of 'which()' is unsigned int
     I know this part is on purpose because the "visible" 'which'
     is not the same as the internal 'which', but at least a comment
     would be appropriate, specially since, for instance, 'assign_into'
takes
     a 'int which' but is being passed 'operand.which()'.
     Anyway, I rather have 'int which()' instead, because I don't like to
use
     unsigned types for numbers that won't be negatives; I use unsigned
types
     IIF I need modulo arithmetic or very large numbers.
     Similarly, apply_visitor_impl<>() takes a runtime argument of type
     'unsigned int' and a compile-time argument of type
     'integral_c<unsigned long,x>. The two are being compared so the
     types should be the same (plain 'int' IMHO)

d) Notes on bcc5.5.1 compatibility:

* Whenever accesing integral constants, make sure to use full
  qualification.

  For example:

    This line,
      , max_size = (max_value< Types, mpl::sizeof_<mpl::_1> >::type::value)
    Must be spelled
      , max_size = ( ::boost::detail::variant::max_value< Types,
mpl::sizeof_<mpl::_1> >::type::value)

  In practice, look for "::value" and make sure it's fully qualified.

  Similarly, local values cannot be used directly, thus, this line:

    typedef aligned_storage<max_size, max_alignment>

  must be rewritten:

    typedef aligned_storage<
::boost::detail::variant::make_storage<Types>::max_size,

::boost::detail::variant::make_storage<Types>::max_alignment
                           >

  In practice, look for BOOST_STATIC_CONSTANT() and if the constants are
used locally,
  add full qualification.

  Another good practice is the following:

  Metafunctions such as:

    is_directly_extractable<>

  usually have a 'bool_c<>' as its "::type", which means that
  you don't really need to access its "::value" and wrap it in a "bool_c<>",
  as in:  "mpl::bool_c<directly_extractable::value>()"
  since that is the same as: directly_extractable::type()"
  Although in the places I've seen this in the variant code the
metafunctions
  are being used at runtime, the valueless form is likely to compile on more
  compilers (bcc551 for instace) if used as a true ct argument.
  Similarly, the new type traits has been extended to expose bool_c<>
directly,
  so you don't need to write:
     mpl::bool_c< is_const<Extractable>::value >()
  and you can write
     is_const<Extractable>::type()
  instead.

--
Fernando Cacciola