Re: [boost] [Review] Quantitative Units library review begins today, March 26

From: Matthias Schabel (boost_at_[hidden])
Date: 2007-03-26 13:29:56

Hi Zach,

Thanks for the review.

> However, I have one strong objection.  What is the rationale behind
> providing the mutating value() member function, and functions
> from_value() and quantity_cast() for converting raw scalar values into
> quantities?  I really like the default rules on construction of

These were requested at various points in the development of the  
library;
I completely agree with you that most of these shenanigans are  
undesirable.
You will note that the mutating value() member function is not used  
anywhere
in the test or example code - I'd be first in line to get rid of it,  
frankly.

> of the quantity's class invariants.  I want to be able to construct a
> set of quantities using their constructors, and thereafter be able to
> know, without checking through the code for abuses of value(), etc.,
> that they and the results of any math done using them, are well-formed
> quantities.  That is, I want Boost.Units to be hermetically sealed as
> much as is possible, so that I know that if I specify the units and
> values correctly at construction, I don't have to think about
> correctness after that.  Mutable value() and quantity_cast(), and to a

Again, I agree. However, you can never get away from having to look for
*_cast in your code if you want to be sure it is abomination free...

> lesser extent from_value(), mean I have to work a lot harder (read:
> look in a lot more places) to ensure that my code is correct.
>
> Specifically,
>
> quantity<force>     F(2.0*newton);
> quantity<length>    dx(2.0*meter);
> F.value() = dx.value();
>
> is an abomination and should be impossible.  If I really want to do

Sure - I'm really happy to remove the mutating value() member function.
The static from_value() member is not strictly necessary, but may be a
reasonably compromise as it doesn't allow "abominations". It is also  
used
in a number of places where we would have to use template friend
declarations otherwise.

> this, why not make me go through the constructor, so that attention is
> more obviously called to what I'm doing?  I could also live with using

I think having a constructor directly take a value_type is the worst  
case
scenario since it would make it very easy to neglect to specify a  
unit, thereby
losing all type-safety gained by the intentionally redundant  
construction
syntax...

> a purposely ugly cast here, but since quantity_cast() has some
> perfectly legitimate and safe uses, I would much prefer a different
> syntax that draws attention to this unsafe act, such as
> quantity_reinterpret_cast().

I'm happy to do this or to restrict value_type construction to the  
from_value()
member.

> Why in the output of quantities are the negative exponents in
> parentheses?  It seems a little odd that negative whole numbers do,
> and positive whole numbers do not.

No particular reason - it seems more readable to me, but I'm not  
wedded to
it... At some point in the future, a more complete and mature system  
including
localization and ability to control output format is on the agenda.

> I and a lot of other users badly need support for Imperial units.  As
> it stands now, we will each be required to write nearly identical code
> to do so.  This should be included in the library.

The problem with these non-standard units is that they don't really  
form a
well-defined unit system. FIrst off, there are Imperial units, US  
customary
units, US survey units, nautical units, and English units - many of  
these have
slightly variant definitions for base values. Because there is no  
specific
standard for the base units (that is, what is the base unit of length  
in the
Imperial system? inch? foot? yard? fathom? mile?), you would need to
essentially have a distinct system tag for each unit, along with a whole
slew of specializations for various conversions. There is an  
undocumented
header file (boost/units/systems/other/non_si_units.hpp) that contains
conversion factors for a wide range of non-SI units to SI. I realize  
that this
is suboptimal for some applications, but I don't think it is possible  
to solve
the problem of Imperial/English/US units in a clean way that will  
satisfy
everyone. We have, however, supplied a number of examples of how to
roll your own unit system...

> You might want to reserve all system ordinal numbers below 1000 or
> 10000 for Boost.Units, instead of stopping at 100.  There are plenty
> more numbers for users to choose from, and though 100 might sound like
> plenty now, you never know how many you might need.

Or maybe the ones from 1000 up?

> to be desired.  Specifically, do I need to assign any of these static
> constants an actual value?  The macro does not do so.  Why does the

The macro does not because it is used both for declaring static constant
units (meter, second, etc...) that only have a default constructor  
and for
static constant quantities that need to be assigned a value. Perhaps we
need two macros for this instead?

> example include singular and plural forms of the unit types ("meter"
> and "meters", etc.)?

Just for syntactic convenience - this way you don't have to remember if
the definitions are singular or plural and it makes the code more  
literate
if you read your equations :

quantity<SI::length>	q1(1.0*meter),
                                        q2(2.0*meters);

>
> In example 1, it looks like you duplicated the first code block.

Actually not - L_T_type and V_type should be the same, so that's what  
we're testing...

> Examples 14, 15, and 18 consist of a single line each.  Was this  
> intentional?

This documentation could be fleshed out more or the examples removed...

> As mentioned in the TODO, there is no concept documentation.  While
> this is not strictly necessary to use much of the library, it should
> at a minimum include concept requirements for quantity::value_type so
> that people can create their own Units-compatible types.

Yes - at some point I will do this. I just didn't want to put in a  
half-baked
concept specification and would like it to be as minimal as possible so
the requirements on value_type are correspondingly minimal...

> * Do you think the library should be accepted as a Boost library?
>
> In its present form, no.  It pains me to say this, because I think the
> library is so good.  If my concern about maintaining quantity
> invariance is addressed, I will happily change my no to a yes.  I also
> would strongly like to see Imperial units supported out of the box,
> though that would not by itself be enough for me to vote no.

As I mentioned above, I'd be happy to eliminate the mutating value()
member function. I am also not personally invested in the quantity_cast
functions or their specific syntax. I would like to retain the static  
from_value()
member function as I am virtually certain that there will be requests to
enable some form of direct construction of quantities from a value_type.
On the other hand, I wouldn't object to retaining some quantity_*_cast
syntax and using that instead of from _value...

Matthias