
-What is your evaluation of the design? It makes sense to me to have a rigid compile-time system for this, the design is simple and very intuitive. I was able to use the basic unit conversions without even reading the docs, and everything shows up just where you expect it. The library does require a lot of typing, but this is going to automatically go away once the auto keyword becomes standard. I do disagree with previous comments on the evilness of the mutating value() method. In particular, if the unit base is a user-defined type, then there is no other way for the user to access the contained object's member functions, so I don't see how you can get away without having value(). (As a reasonable example, you might want to use units with a boost::rational<int> data type, but without a mutating value(), you can't call boost::rational<int>::assign() anymore). I think this deficiency would cause people either to not use units, or to use a quantity<Q, boost::reference_wrapper<T> > instead, which is annoying and suffers from the same dangers anyway. Also, quantity<> should pass through operator++ and operator-- in addition to the other arithmetic operators. I was surprised to find that the prefixes kilo, mega, etc, are in namespace SI. They are equally useful for CGS and for user defined systems; perhaps they should be in a new namespace boost::units::prefix, since they are just static constants? Also, I was surprised that units/systems/si/prefixes.hpp was not included by units/systems/si.hpp, since I thought that was going to be a "catch-all" header. -What is your evaluation of the implementation? I didn't look in a lot of detail, enough to tell that there is good re-use of Boost metaprogramming libraries, etc, and the code seems easy to follow. It compiles in a reasonable amount of time. One thing I noticed is that the code does not use BOOST_STATIC_CONSTANT. Is this under the assumption that compilers which need BOOST_STATIC_CONSTANT won't compile the code anyway? That's probably correct I guess. I was surprised by the following behavior: #include <iostream> #include <boost/units/io.hpp> #include <boost/units/systems/si.hpp> #include <boost/units/systems/cgs.hpp> #include <boost/units/systems/si/prefixes.hpp> using namespace boost::units; int main() { quantity<CGS::force> F0 = 20 * CGS::dyne; quantity<SI::force> F1 = quantity_cast<SI::force>(F0); quantity<SI::force> F2 = quantity_cast<SI::force>(20 * CGS::dyne); quantity<SI::force> F3(F0); //quantity<SI::force> F4 = F0; //quantity<SI::force> F5(20 * CGS::dyne); //quantity<SI::force> F6 = 20 * CGS::dyne; std::cout << F1 << '\n' << F2 << '\n' << F3 << '\n'; } Firstly, I would have expected all 6 initializations of F1-F6 to do exactly the same thing. The last three do not compile at all, though. I think it is particularly surprising that there is a difference between F3 and F4. Most users are used to these two syntaxes being absolutely identical in meaning. I haven't looked at the implementation in detail, I'm just pointing out that this is a surprising feature. Also problematic is F2, which does compile, albeit with a warning, but then does the wrong thing at run time. The output of this program is: 1e-05 m kg s^(-2) 0 m kg s^(-2) 1e-05 m kg s^(-2) This appears to be related to the fact that in the initialization of F0, there is an implicit conversion from the int (20) to double. If I change the int to a double (20.) , then the code compiles without warnings and does the right thing. Similarly, changing the int to a double allows F5 to compile, but again, the equivalent-looking F6 does not. BTW, the warning issued by gcc is: ../../../boost/units/conversion.hpp: In static member function `static boost::units::quantity<boost::units::unit<Dim1, boost::units::homogeneous_system<S2> >, Y> boost::units::conversion_helper<boost::units::quantity<boost::units::unit<Dim1, boost::units::homogeneous_system<S> >, Y>, boost::units::quantity<boost::units::unit<Dim1, boost::units::homogeneous_system<S2> >, Y> >::convert(const boost::units::quantity<boost::units::unit<Dim1, boost::units::homogeneous_system<S> >, Y>&) [with S1 = boost::units::CGS::system_tag, S2 = boost::units::SI::system_tag, Dim1 = boost::units::force_type, Y = int]': ../../../boost/units/quantity.hpp:91: instantiated from `boost::units::quantity<Unit, Y>::quantity(const boost::units::quantity<boost::units::unit<Dim2, System2>, YY>&, typename boost::disable_if<typename boost::units::is_implicitly_convertible<boost::units::unit<Dim2, System2>, boost::units::unit<typename boost::units::get_dimension<T>::type, typename boost::units::get_system<T>::type> >::type, void>::type*) [with System2 = boost::units::CGS::system, Dim2 = boost::units::force_type, YY = int, Unit = boost::units::unit<boost::units::force_type, boost::units::SI::system>, Y = int]' ../../../boost/units/quantity.hpp:360: instantiated from `boost::units::quantity<boost::units::unit<Dim, System>, X> boost::units::quantity_cast_helper<boost::units::unit<Dim, System>, boost::units::quantity<Unit2, X> >::operator()(const boost::units::quantity<Unit2, X>&) [with X = int, System = boost::units::SI::system, Dim = boost::units::force_type, Unit2 = boost::units::unit<boost::units::force_type, boost::units::CGS::system>]' ../../../boost/units/quantity.hpp:378: instantiated from `typename boost::units::quantity_cast_helper<X, Y>::type boost::units::quantity_cast(const Y&) [with X = boost::units::SI::force, Y = boost::units::quantity<boost::units::unit<boost::units::force_type, boost::units::CGS::system>, int>]' t.cpp:14: instantiated from here ../../../boost/units/conversion.hpp:52: warning: passing `double' for converting 1 of `static boost::units::quantity<Unit, Y> boost::units::quantity<Unit, Y>::from_value(const Y&) [with Unit = boost::units::unit<boost::units::force_type, boost::units::SI::system>, Y = int]' -What is your evaluation of the documentation? The large number of examples is excellent. The documentation needs a lot more substance to it, particularly explaining surprising features of implicit conversions. -What is your evaluation of the potential usefulness of the library? I can see it being very useful. I myself am considering using it in the near future. One concern I have, is that I use a lot of C++ wrappers over legacy C libraries such as CLAPACK and ATLAS. I would still be able to use boost::units, provided I could count on sizeof(quantity<Q,T>) == sizeof(T), so that an array of quantitity<Q,T> would look the same in memory as an array of T. I doubt you would find any compiler where this is not the case, but it would be nice if the library could offer some sort of guarantee, since the standard doesn't, or at least comment on this issue in the docs. Another convenient option would be to put a static assert in the definition of quantity<> that the user could turn on via a #define if they want to be sure of this. -Did you try to use the library? With what compiler? Did you have any problems? The compile of cmath fails on gcc 3.4.4 on cygwin/MinGW and on gcc 3.4.6 on Linux because of undefined symbol __builtin_signbit in detail/cmath_gnu_impl.hpp line 195. This prohibits eg compilation of unit_example_11.cpp There are also a few source files missing terminating newlines, which produces a warning on some compilers. The library does not compile on earlier gcc versions as I mentioned in an earlier email, but this is not surprising. Other than cmath, the library compiled fine on gcc 3.4.{4,6} and 4.0.2. gcc 3.4.{4,6} was not able to optimize the runtime overhead entirely away. Without optimizations, the supplied timing example was about 10 times slower compared to the non-unit version. With optimization, it was only about 20% slower. On gcc 4.0.2, with optimization it was the same speed. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Played around with it for a couple hours, didn't put in a lot of time reading the actual implementation. -Are you knowledgeable about the problem domain? Yes, I am knowledgeable about units/dimensional analysis. - 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. Yes, I think this library is a good and useful example of compile-time programming, and it should be accepted. I do have some concerns about the implicit conversions and initialization issues, but I think they probably just stem from my lack of understanding of some implementation issues, which means that improved docs would probably suffice to take care of them. I do think the one issue I mentioned above (the combination of quantity_cast and a conversion from int to double producing incorrect runtime behavior) qualifies as a bug, perhaps compiler-specific? -Lewis