Re: [boost] units: review

From: Matthias Schabel (boost_at_[hidden])
Date: 2007-03-28 21:26:03

Lewis,

First off, thanks for the thorough review and for pointing out some  
bugs that have crept in - I'll make sure to add these test cases to  
the unit tests so they are caught as they should have been.

> 	Also, quantity<> should pass through operator++ and operator-- in
> addition to the other arithmetic operators.

I debated this for a while - the only problem is that there will be  
some value types with heterogeneous operator+ / operator- for which q  
+ 1 is not the same type as q, but I guess that it is just OK for it  
to just give a compile error in those cases.

> 	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.

Good suggestions.

> 	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;
>
> 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

You've found a couple of bugs that crept in after some rewriting of  
the code to make it a little more terse. F6 is not expected to  
convert because implicit unit system conversions are prohibited while  
explicit unit system conversions are allowed. The other five cases  
should be identical, as you expect.

> 	The large number of examples is excellent. The documentation needs a
> lot more substance to it, particularly explaining surprising  
> features of
> implicit conversions.

The implicit/explicit conversion rules are documented, but, as they  
can be surprising, should probably be more front and center.

> 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

Unfortunately, I believe that there is no way to guarantee this since  
quantity isn't a POD type. However, we certainly could put in a  
compile-time assert on sizeof like you suggest...

> 	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

I forgot to skip these cases on MinGW - didn't know that it was a  
problem under Linux. Maybe I'll just remove the signbit part in the  
example since the support is spotty anyway.

> 	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?

Thanks for your vote and, more importantly, for the close look. We'll  
fix the bugs ASAP.

Matthias