Re: [boost] Conversion review

Subject: Re: [boost] Conversion review
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2011-08-20 08:41:23

Hi,
Le 20/08/11 12:03, Jeroen Habraken a écrit :
> My review of the Conversion 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.)
Thanks for the suggestion. I will take it into consideration if the 
library is accepted.
>   * 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.
OK.
>   * 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.
We could choose the string representation that the C++ langage uses.
> - 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.
I respect your approach, but I have see tones of code using these kind 
of loops that cluter the comprehension of the code. I prefer to use a 
function. Nothing forces you to use this function.
> 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.
I don't care about the beautifuness of the library code but about the 
user code. How could you define a conversion between pairs of types that 
are extrinsically convertibles without having a generic function?
> - 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.
I would prefer if the Boost libraries could define these constructors 
without a generic extrinsic conversion function.
>   * 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.

I have compiler with clang 2.9 with and without c++0x support and I have 
not find this error. With which Boost version are you compiling? Is this 
related to the recent introduction of segmented sequences?
>   * 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.
Do you have a suggestion on which form it could?
> 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.
I have already proposed to John and Joel to work on this. I'm ready to 
move in tis direction independently of the result of the review.
> - 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.
I'll make a feature request ticket.

Thanks for your review,
Vicente