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
Hi, Le 20/08/11 12:03, Jeroen Habraken a écrit : 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