[boost] Conversion review

Subject: [boost] Conversion review
From: Jeroen Habraken (vexocide_at_[hidden])
Date: 2011-08-20 06:03:44

My review of the Conversion library:

Please note that I'll be referring to the pre-review thread which can
be found here: http://lists.boost.org/Archives/boost/2011/07/183525.php.

 * What is your evaluation of the design?

Please refer to the section discussing the potential usefulness of this library.

 * What is your evaluation of the implementation?

The code looks clean but I'm worried about the abundant use of
boost::enable_if. While it is a useful tool SFINAE does make code
fragile, possibly using something akin the following is an option:

template <typename Target, typename Source, bool CA, bool EEC, bool A>
struct assigner_impl {
    // No cookies
    BOOST_STATIC_ASSERT(sizeof(Target) == 0);
};

template <typename Target, typename Source>
struct assigner_impl<Target, Source, true, true, false> {
    // Copy-assignable, extrinsically explicitly convertible and not assignable
    ...
};

template <typename Target, typename Source, bool CA, bool EEC>
struct assigner_impl<Target, Source, CA, EEC, true> {
    // Assignable
    ...
};

template <typename Target, typename Source>
struct assigner
    : assigner_impl<
            Target,
            Source,
            is_copy_assignable<Target>::value,
            is_extrinsically_explicitly_convertible<Source, Target>::value
            is_assignable<Target, Source>::value
    > { };

(Note, this was written on the fly in this email, it may not actually compile.)

 * What is your evaluation of the documentation?

Everything seems to be covered well, small comment though:

template <>
struct is_assignable<X&, X const&>
    : true_type { };

I initially missed that true_type is boost::true_type, this should be
made more obvious.

 * What is your evaluation of the potential usefulness of the library?

Let me tackle this in several parts:

- type-to-string and string-to-type conversions

In the pre-review we came to the following conclusions:

"Coming back to the string to int conversion, if we can not have a default
string-to-type or type-to-string, Boost.Conversion must remove these
specializations."

"I don't think that Boost.Conversion can provide a string-to-type conversion
that is configurable. It could provide one if there is a consensus of a
default. We should see Boost.Conversion customization as if we were adding
some methods to one of the classes. For the string to float conversions, it
is as if we were adding

explicit operator float() const;

to the std::string class.
"

I still maintain that this is the case, there is no sane way to let
the user select between Convert or LexicalCast at the moment and
possibly Coerce in the future and it's not something this library
should be tackling.

- STL conversions

I believe these functions are of little use as when I would have to
convert a list of integers to a list of doubles I'd simply write a
loop (possibly aided by BOOST_FOREACH) instead of looking up a library
(and reading the necessary documentation) to do it for me.

With the offered customization points the cure seems to be worse than
the disease, if I had to convert a std::pair<int, int> to a
std::pair<double, double> I'd write a quick function to do so. The
Conversion documentation however shows the following:

template <typename Target1, typename Target2, typename Source1,
typename Source2>
struct implicit_converter_cp< std::pair<Target1,Target2>,
std::pair<Source1, Source2>
  , typename enable_if_c<
        is_extrinsically_convertible<Source1, Target1>::value
    &&  is_extrinsically_convertible<Source2, Target2>::value
  >::type
> : true_type
{
    std::pair<Target1, Target2> operator()(std::pair<Source1, Source2>& v)
    {
        return std::pair<T1, T2>(implicit_convert_to<T1>(from.first),
implicit_convert_to<T2>(from.second));
    }
};

This is a more generic solution but it seems like an awful lot of
trouble for something this simple.

- The Boost conversions

These do seem very useful, but as discussed in the pre-review:

"You are right and the same applies to any type-to-type conversion. And maybe
this is the MAJOR flaw of Boost.Conversion: to think that we can define
extrinsically THE conversion between two types. Every body admit that if a
class T defines a constructor from a std::string THE conversion from
std::string to T is the one defined by this constructor."

I do believe that this library isn't the place for the conversions,
they should be added to the specific libraries themselves in the form
of constructors.

 * Did you try to use the library? With what compiler? Did you have
any problems?

I've tried several examples with GCC 4.2.1 (the default shipped with
OS X), 4.5.3 and clang 2.9 on OS X.

Both versions of GCC worked fine, clang however was unable to compile
the examples with the following error:

In file included from no_throw.cpp:13:
In file included from ../../../boost/conversion/include.hpp:20:
In file included from ../../../boost/conversion/convert_to.hpp:22:
In file included from ../../../boost/conversion/explicit_convert_to.hpp:38:
In file included from ../../../boost/conversion/implicit_convert_to.hpp:40:
In file included from
../../../boost/conversion/type_traits/is_convertible.hpp:25:
In file included from ../../../../library/boost-trunk/boost/fusion/tuple.hpp:10:
In file included from
../../../../library/boost-trunk/boost/fusion/tuple/tuple.hpp:11:
In file included from
../../../../library/boost-trunk/boost/fusion/container/vector/vector.hpp:11:
In file included from
../../../../library/boost-trunk/boost/fusion/container/vector/detail/vector_n_chooser.hpp:14:
In file included from
../../../../library/boost-trunk/boost/fusion/container/vector/vector10.hpp:14:
In file included from
../../../../library/boost-trunk/boost/fusion/sequence/intrinsic/begin.hpp:17:
In file included from
../../../../library/boost-trunk/boost/fusion/sequence/intrinsic/detail/segmented_begin.hpp:11:
In file included from
../../../../library/boost-trunk/boost/fusion/iterator/segmented_iterator.hpp:13:
In file included from
../../../../library/boost-trunk/boost/fusion/container/list/cons.hpp:14:
In file included from
../../../../library/boost-trunk/boost/fusion/sequence/intrinsic/end.hpp:17:
../../../../library/boost-trunk/boost/fusion/sequence/intrinsic/detail/segmented_end.hpp:33:70:
error: invalid use of incomplete type 'fusion::nil'
                segmented_end_impl<Sequence, fusion::nil>::call(seq,
fusion::nil()));

^~~~~~~~~~~~~

Not sure what's going on here exactly.

* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

Something between a quick reading and an in-depth study.

 * Are you knowledgeable about the problem domain?

As the author of Coerce I'm knowledgeable about the string-to-type and
type-to-string domain, less so about the generic type-to-type domain
though.

 * Do you think the library should be accepted as a Boost library? Be
sure to say this explicitly so that your other comments don't obscure
your overall opinion.

NO, the library should not be accepted as a Boost library in the current form.

However the following should be done:

- There are several type traits which can possibly be added to
Boost.TypeTraits. This is not something I can judge, it is up to the
auteurs of Conversion and TypeTraits to judge which are useful.
- The conversions regarding Boost classes should be added to the
respective libraries in the form of constructors if the respective
auteurs believe this adds value to their library.

Kind regards,
Jeroen Habraken