[boost] units: review

From: Lewis Hyatt (lhyatt_at_[hidden])
Date: 2007-03-28 18:53:54

-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