This review comes a bit late, because I'm really short on time this week and I still wanted to study the library as close as possible. I plan incorporate units into a fairly big project (http://yade.berlios.de/ it's big: over 15000 lines). Before I do so I need to check if it fulfils my requirements. (a) must work with Vector3 and allow working with (dimensionless) Quaternions (inflicting rotation (radians/degrees <-> quaternions) and calculating momentum [N/m] (b) must allow GUI input with unit selection by the user (predefined dimensions) (c) the docs must be good enough for me to find the answer quickly The (a) requirement is satisfied as can be seen in the the examples, so due to lack of time I didn't bother to check that in-depth. The (b) caused tons of discussions and, if I'm correct, a negative vote from Noah Roberts. I had no time to read carefully that discussion thread to be 100% sure that my requirement is exactly the same as the issue raised there. I've written a short program to see if it's possible to make. And surprisingly it was very easy and intuitive to write. Perhaps the library authors will want to include this into their examples. The attached screenshot and the attached sources should make my intentional usage clear. See section 5 below for detailed discussion. Correct me, if that is not what Noah requested. The most relevant code is in the attached calculatorform.cpp file. Other files are created by qt4. The (c) is answered in section 3. Bottom line: good design solved the problem of lacking documentation. (not much need to look at docs if everything is intuitive) 1. What is your evaluation of the design? ================================================= Very intuitive. Much more intuitive than most of the other libraries that are currently in boost. Maybe because as a C++ programmer and an engineer I know how units should work - and the interface presented was nearly as I would expect it to be. I've run into this library at full speed and I didn't bump into any wall of misunderstandings. Since I wanted to write my testing (b) program as quick as possible it was close to the "real anger" test, and the outcome was positive. The good design was the major factor that helped with that. With good design I didn't need to spend unnecessary minutes to search for an answer in the documentation. 2. What is your evaluation of the implementation? ================================================= My general impression is that the implementation is close to optimal in terms of template design. I looked at the sources rather briefly, so I won't comment about implementation details. I can only speak about the "outside impression". My only complaint is about lengthy typenames. Like for example my Newton's Law function (attached calculatorform.cpp:65): quantity<SI::force> newtons_law(const quantity<SI::mass>& m, const quantity<SI::acceleration>& a) { return m*a; } I'd prefer a shorter variant: force newtons_law(const mass& m, const acceleration& a) { return m*a; } Of course I can make a typedef. But the name "force" is already occupied. Using name "force_type" (as proposed in the examples) is a bit inconsistent, because the SI::force is a type too. And "force_type" is longer, which I don't like ;) That's just a small usage glitch, and I'm 100% sure that it can be solved this way or another. As it's just about naming. If the library remains unaltered I'll simply make some short typedefs and use them thorough 15000 lines of my code ;) A good practice BTW. PLURALS As mentioned in other posts the ISO 31-0:1992 tells that unit names should not be plural. I was unable however to see the ISO standard itself (google only brings up the webpages where I can *buy* the ISO standard), but I have found an ISO faq about that: http://lamar.colostate.edu/~hillger/faq.html#spelling-names and (search the text for "plural"): http://www.unece.org/trade/untdid/download/r1224.txt (I think that "unaltered in plural" means that they have no plural) I (like others) think that users will prefer to have less options and use whatever the library authors will decide. Therefore I suggest to drop the plurals. ISO standard however applies only to SI units, other systems, because they are non-standard, may need to use plural ("feet" being an outstanding example). Therefore I leave that issue up to the authors. 3. What is your evaluation of the documentation? ================================================= Documentation is a bit lacking. But in fact I didn't want to spend much time reading it. I wanted to quickly go into 'coding mode' - so that was a kind of "real anger" test. And the library was coded so intuitively with attached examples so complete, that I rarely needed to look into documentation. However when I had to look there I noted following things: "Example 14" : I suggest to put there at least the program's output there. Same with Example 15 and 18. The "base_unit_converter<>" in Example 16 should be hyperlinked to the explanations what it is and how to use it. And similarly all the other keywords related with the library. People like me in "real anger" spend most of the time looking at examples. Therefore examples should be heavily hyperlinked to the rest of the documentation, so that every keyword has explanation under a single mouse click. Documentation should have a FULL list of all predefined units, so it will be a quick reference for the users. 4. What is your evaluation of the potential usefulness of the library? ================================================= Huge potential. In the past I've spent countless hours looking for an error that such library would help to avoid in the first place. Very useful. 5. Did you try to use the library? With what compiler? Did you have any problems? ================================================= COMPILERS I urge the authors to add some basic compiler-checking and emit an #error when an unsupported compiler is used. Due to lack of time I did not investigate why tutorial.cpp did fail on: gcc version 3.3.5 (Debian 1:3.3.5-13) (sarge), but instead I quickly switched to: gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21) (etch) Perhaps it was my fault. But IMHO not: g++ 3.3.5 is very old and I'd be surprised if it worked. So some #error (or a #warning at least) from the library would be nice to make it clear for the people. EXAMPLES Speed "Example 14": With debug information and no optimizations: f(x,y,z) took 8.04 seconds with double f(x,y,z) took 108.64 seconds with quantity<double> g(x,y,z) took 8 seconds with double g(x,y,z) took 91.07 seconds with quantity<double> With full optimization g++-4.1 -O3: f(x,y,z) took 2.71 seconds with double f(x,y,z) took 2.7 seconds with quantity<double> g(x,y,z) took 2.72 seconds with double g(x,y,z) took 2.7 seconds with quantity<double> Surprise: with units it's faster! How could that be possible? Perhaps a measurement error. I increased REPETITIONS tenfold: f(x,y,z) took 28.11 seconds with double f(x,y,z) took 28.12 seconds with quantity<double> g(x,y,z) took 30.3 seconds with double g(x,y,z) took 28.13 seconds with quantity<double> Well, I don't get it. But I like it. I thought that AMD processor may cache something and the second invocation of f() is faster so I changed the order of function calls: f(x,y,z) took 27.08 seconds to run 1e+10 iterations with quantity<double> = 1.4771e+09 flops f(x,y,z) took 29.18 seconds to run 1e+10 iterations with double = 1.3708e+09 flops g(x,y,z) took 27.09 seconds to run 1e+10 iterations with quantity<double> = 1.47656e+09 flops g(x,y,z) took 29.18 seconds to run 1e+10 iterations with double = 1.3708e+09 flops With units it is faster by two seconds. I ran the test several times, and read the code few times. Maybe executable with "slower" units is accidentally better synchronized with memory access frequency MHz. Congratulations, you beat me. Those results were obtained with: gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21) THE GUI INPUT UNITS TEST This short QT4 program in the attachment was my only experience with this library, and a very positive one. (apart from compiling attached library examples) If you want to compile it, you need qt4 installed, then you can run (linux, mac, windows): qmake make and you should get an executable. The part most relevant to proposed units library is in calculatorform.cpp file. I have created a dropdown menu (QWidget::ComboBox) where the user can pick the desired unit of the input. Each item in this drop down menu is given a conversion factor. In current implementation only few conversion factors are present and they are hardcoded. However in the real world scenario the conversion factors will be put in some array, and will be configurable by the user from the software's GUI (like File->Preferences). This would make the attached example shorter by few lines, and looking much more clean. By writing this example I am absolutely sure that it is simple to make a customized unit input. And the reader should be convinced too, after a short look at the attached .cpp file. The design decision of the library authors to skip the unit input, was the best possible. Because otherwise the library would have to support everything possible: qt4, wxwidgets, gtk, winapi, iostreams, etc.... That would be crazy :) Better provide right tools, and the user (me) does the work he needs, like in the attached code. However I encountered a small glitch there. I couldn't obtain a conversion factor from simple units division, which is surprising (calculatorform.cpp:36) force_unit_box->addItem("dyne", 1.0*CGS::dyne/SI::newton ); dividing '1 dyne' by '1 newton' produces a dimensionless double value that should contain the conversion factor, but instead it wrongly produces "1.0". Finally I had to hardcode the conversion factor. I also tried to look for base_unit_converter<> but there wasn't any defined for force, and I wouldn't like this solution at all. I expect that this issue will be resolved before adding the library into boost. The best solution is to allow implicit conversion from dimensionless type to underlying type (double), as in calculatorform.cpp:47. 6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? ================================================= It was two evenings, about 6 hours each. Would give 12 hours total. Much more than quick reading. But not an in-depth study. 7. Are you knowledgeable about the problem domain? ================================================= Yes, I've written a PhD in civil engineering, and I'm programming in C++ for 10+ years. 8. Do you think the library should be accepted as a Boost library? ================================================= Yes it should be accepted. Before adding to CVS-HEAD please do following: 1. support calculations of conversion factors as explained above. Attached file calculatorform.cpp:47 (or convince me that I'm wrong, but hey - how to do it more conveniently than in this way?) 2. check for compiler used and emit an #error when unsupported compiler is detected. So that users won't waste time on futile fights with their compilers. -- Janek Kozicki |
