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

From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2007-03-26 12:07:31

* What is your evaluation of the design?

Overall, the library design is very good.  I particularly like the
fact the user-defined value types, std::complex, etc., work so
effortlessly with the library.  The support for and integration of
trig and cmath functions is great as well.

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
quantities.  "quantity<force> F(2.0*newton);" is clear, unabmiguous,
and literate.  The holes in this system opened by the above functions
make me less certain of the library's safety.  The mutating value()
function is particularly egregious.  It allows the arbitrary violation
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
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
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
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().

* What is your evaluation of the implementation?

Very good.  The code is very easy to read and follow, and there seems
to be attention paid to all the usual gotchas, like ODR and ADL.

Some specifics:

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.

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.

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.

* What is your evaluation of the documentation?

It's a good start :).  The documentation is clear and relatively easy
for me to follow.  However, there needs to be more discussion of
concepts and how to extend the library by adding one's own value
types, units, and systems.

Here are some specific notes about the docs:

In the Units intro text, I believe in the sentence, "Units are, like
composite dimensions, purely compile-time variables with no associated
value.", variables should be entities.  Units are not variables per
se.

The text in Units that reads "The macro A convenience macro that
allows definition of static constants in headers in an ODR-safe way.
..." should instead read "". The macro BOOST_UNITS_STATIC_CONSTANT
...".  Also, the description of what this macro does leaves something
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
example include singular and plural forms of the unit types ("meter"
and "meters", etc.)?

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

It looks like the (unit_example_*.cpp) links are broken for all
examples, though I only tried the first 5.  There appear to be lots of
other non-links that are underlined and that look like they should be
links, for instance power_typeof_helper and root_typeof_helper in
several places.

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

In example 19, the link to boost/units/systems/si/codata_constants.hpp
brings up a basically empty page.

In the reference page, several cmath.hpp and io.hpp are missing their
contents (functions, macros, etc.).

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.

* What is your evaluation of the potential usefulness of the library?

*Extremely* useful.  The ability to have dimensional analysis and unit
converiosn safety checked by the compiler instead of by hand is useful
to anyone that does scientific computing.

* Did you try to use the library?  With what compiler?  Did you have
any problems?

Yes.  GCC 4.1.0 / Linux.  No problems with any of the examples I
compiled.  I did not compile all of them, nor did I build the unit
tests.

* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

A quick look at the implementation (not all files), a thorough reading
of the docs, and a bit of coding, based on the examples.

* Are you knowledgeable about the problem domain?

Yes.  I've been using Andy Little's PQS library for years, and do a
lot of physics-based programming in general.

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

Zach Laine