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
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
A note to an earlier message before I start my review:
- 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. There's nothing easier than converting a list of integers to a vector of doubles: std::list<int> li; // fill list with something ... std::vector<double> vd( li.begin(), li.end() ); Concerning std::pair<int,int> to std::pair<double,double> conversion: There's already a template constructor in the std::pair class which does that.
My review: * What is your evaluation of the design? Looks like good design. * What is your evaluation of the implementation? I didn't look into it. * What is your evaluation of the documentation? Looks quite accurate. For my taste the documentation is a litte to verbose. For example the bullet points in "How to Use This Documentation" can be left out in my opinion. There were a few typos: "Unfortunately this doesn't work<s> as overload resolution doesn't take care of template type parameters that can not be deduced from the function arguments. *Boost.Conversion* provides a customization <customization> point that takes in account the |Source| and the |Target| types (see below). " * What is your evaluation of the potential usefulness of the library? I don't like implicit conversions in the first place. This hasn't anything to do with the library itself. But providing implicit conversions can create temporary objects or make a program behave weird in the strangest places. Sometimes versions of overloaded functions are called, you wouldn't expect. Some stuff compiles, where you would rather have a compile time error than a runtime crash. I try to avoid implicit conversions and prefer handwritten conversion where neccessary. This way you will always know what's happening in your code. Therefore the library would not be of great use for me. I'm not against implicit conversions per se, but from my experience they should be used with great care. Using this library might suggest the opposite to the unexperienced user. * Did you try to use the library? With what compiler? Did you have any problems? No I didn't try the library. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I read the overview and had a glance at the tutorial. * Are you knowledgeable about the problem domain? It's a rather basic domain. As any professional C++ programmer would be. * 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. Explicit conversions might be fine, but implicit conversions should be used with great care and should mostly be avoided. I mentioned reasons for that in the answer about usefullness. Explicit conversion can be done by handwritten functions without studying another library. Best regards, Ralph.
Le 23/08/11 11:20, Ralph Tandetzky a écrit :
A note to an earlier message before I start my review:
- 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. There's nothing easier than converting a list of integers to a vector of doubles: std::list<int> li; // fill list with something ... std::vector<double> vd( li.begin(), li.end() ); Concerning std::pair<int,int> to std::pair<double,double> conversion: There's already a template constructor in the std::pair class which does that.
My review:
* What is your evaluation of the documentation? Looks quite accurate. For my taste the documentation is a litte to verbose. For example the bullet points in "How to Use This Documentation" can be left out in my opinion. There were a few typos: "Unfortunately this doesn't work<s> as overload resolution doesn't take care of template type parameters that can not be deduced from the function arguments. *Boost.Conversion* provides a customization <customization> point that takes in account the |Source| and the |Target| types (see below). "
OK. I will correct it.
* What is your evaluation of the potential usefulness of the library? I don't like implicit conversions in the first place. This hasn't anything to do with the library itself. But providing implicit conversions can create temporary objects or make a program behave weird in the strangest places. Sometimes versions of overloaded functions are called, you wouldn't expect. Some stuff compiles, where you would rather have a compile time error than a runtime crash. I try to avoid implicit conversions and prefer handwritten conversion where neccessary. This way you will always know what's happening in your code. Therefore the library would not be of great use for me. I'm not against implicit conversions per se, but from my experience they should be used with great care. Using this library might suggest the opposite to the unexperienced user. The library provides implicit and explicit conversions, and as you have surely remarqued you need to use an explicit function to get them (implicitly), so the conversion is not realy implicit. No risk for the unexperienced user as it need to state explicitly that he want implicit conversion ;-)
* 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. Explicit conversions might be fine, but implicit conversions should be used with great care and should mostly be avoided. I mentioned reasons for that in the answer about usefullness. Explicit conversion can be done by handwritten functions without studying another library.
I can understand that you could prefer handwritten functions to make explicit conversions. The library helps you to make these handwriten functions to the point you will not need them (the hadwritten) anymore, just include a specific file. Thanks for your review, Vicente
Am 23.08.2011 18:56, schrieb Vicente J. Botet Escriba: >> I don't like implicit conversions in the first place. This hasn't >> anything to do with the library itself. But providing implicit >> conversions can create temporary objects or make a program behave >> weird in the strangest places. Sometimes versions of overloaded >> functions are called, you wouldn't expect. Some stuff compiles, where >> you would rather have a compile time error than a runtime crash. I >> try to avoid implicit conversions and prefer handwritten conversion >> where neccessary. This way you will always know what's happening in >> your code. Therefore the library would not be of great use for me. >> I'm not against implicit conversions per se, but from my experience >> they should be used with great care. Using this library might suggest >> the opposite to the unexperienced user. > * What is your evaluation of the potential usefulness of the library? > The library provides implicit and explicit conversions, and as you > have surely remarqued you need to use an explicit function to get them > (implicitly), so the conversion is not realy implicit. No risk for the > unexperienced user as it need to state explicitly that he want > implicit conversion ;-) Sorry, my fault. I didn't look into deeply enough. Best regards, Ralph.
participants (3)
-
Jeroen Habraken -
Ralph Tandetzky -
Vicente J. Botet Escriba