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

From: Steven Watanabe (steven_at_[hidden])
Date: 2007-03-26 14:11:16

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