AMDG I apologize for replying to a digest. I couldn't find this message on gmane. "Zach Laine" wrote:

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

2.0 * newton, quantity_cast<quantity<newton> >(2.0), and quantity<newton>::from_value(2.0) are equivalent. From the standpoint of safety I don't see why one method is better than another. However, it would be better to have a single method of constructing quantities.

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.

quantity::value() bypasses the protection of the interface. If you use it you have to be careful. One use for the mutable value is modf. The second parameter is used for output. There needs to be some way to implement such functions without having to add friend declarations to quantity.... template<class Unit, class Y> inline quantity<Unit, Y> modf(const quantity<Unit, Y>& q1, quantity<Unit, Y>* q2) { using std::modf; typedef quantity<Unit,Y> quantity_type; return quantity_type::from_value(modf(q1.value(), &q2->value())); } Presumably you will only use value() to call some function that operates directly on the value type.

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

That is one possibility. Let's see what other reviewers think. As I see it this is not an issue of functionality but of syntax.

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

Thanks.

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.

Ok.

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.

Possibly. 100 is about 10 times what we are actually using now.

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

Ok.

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

BOOST_UNITS_STATIC_CONSTANT doesn't belong in the documentation at all. It's an implementation detail.

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.

Yeah. I'll have to figure out how quickbook handles links.

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

Mathias?

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

The header only contains #includes which are stripped by doxygen/boostbook. Thanks for pointing this out.

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. Basically, the ability to bypass the safe interface is sometimes necessary. I am willing to use some kind of uglified syntax that is easy to search for. (like quantity_reinterpret_cast as you suggested.) 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

Thank you for your review. In Christ, Steven Watanabe