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