"Pavel Vozenilek" wrote
"fred bertsch" wrote:
The review of Andy Little's Physical Quantities System begins today,
I do abstain in this voting.
The documentation could be improved a lot. I didn't spotted problems in the code, the tests compile (with tiny fix [13]) and the usability of the library looks within reach of normal developer.
/Pavel
_______________________________________________ 1. The name "pqs" should be changed to something a little bit more descriptive.
"unit" feels quite handy.
Thanks for the suggestion. I am OK with changing the namespace to unit if it makes more sense to do so. ______________________________________________
2. intro.html: I find some terms as "t1_quantity" rather annoying. What they mean? How do these names help novice to understand the library better?
I would recommend to avoid the t1, t2, ... nomenclature. I know about few projects named tX, it is better to avoid such associations.
I have had similar feedback regarding the name t1_quantity. The problem is that there is potentially a family of 3 different types representing quantities and it is not easy to express their individual characteristics together with some expresion of their similarities. The latest I could come up with has been chico_quantity, harpo_quantity and groucho_quantity. At least it gets away from the numbers. In source code, the expected useage is to use the typedefs ( pqs::length::m etc), in the same way that std::string is a typedef for std::basic_string<...>. t1_quantity will come up in error messages and be used for generic functions though.
The funny "physical_quantity" uses underscore intentionally? Shoudn't italics or different font be used?
The idea is that its meant to be a keyword. Maybe that should be 'physical quantity' or 'physical-quantity' though. I once used bold italic for all hyperlinked keywords in a document, but the reviewer reported that they gave him a headache. I will have to experiment with fonts to see whats best.
What may be implemented in the future should be moved out of introduction. A novice will have enough of troubles to grasp what's available now.
Originally, I wished to avoid the library becoming a framework ( where by framework I mean that it requires its own supporting classes), however what I have discovered in the course of writing the library is that you cant implement stronger type checking and expect the library to operate with traditional libraries(such as matrix libraries, complex and so on). The only way to do that is to remove the type checking. Therefore I think its important to stress in the introduction that what is here would only be a small part of a much larger framework.
Marketing ala "Increases programmer satisfaction and enhances productivity" should be left out. It is up to the user to decide usability of the library.
OK. _______________________________________________
3. definitions.html: ERD abbrev should be spelled fully. Smaller and more focused parent - child diagrams could be added for every term, instead of pointing to one huge and hard to grasp quickly graph.
The detailed description of terms should start from the basics and the most simple and continue toward more complex terms - the lexicographic ordering is not that helpful. The ordered list on top is OK.
Every single term should have example or link to example. It is too information dense as it is now and it doesn't give enough of clues to novices.
There is definitely a problem with the emphasis that is placed on the whole definition of terms section. The idea behind putting it first is to let the reader know it is there. The intent was that the reader would notice it, but skip it and only refer to it when necessary. This is one downside of using Quickbook. By default It tends to impose a sequential style ( with the prev/ next arrows), whereas in my hyperlinked documents I prefer to use a 'star' and or hierarchical system with the index as the hub and directories. The entity relationship diagram could be nested to more detailed areas as you suggest and might serve as the basis for a much more graphics based approach generally.
I was not able to understand the last paragraph in base_dimension section.
The idea of a base_dimension is that it isnt reduceable. For example length is a base dimension. However it is possible to have a physical quantity of length to power 1/2. A philosopher can use this to pick holes in the idea that length is irreducible. This is a note to say that the choice of base-dimensions is rather 'loose' and not to be taken too seriously. They are accepted practical conventions only.
It shlould be always spelled what every term means in C++: class, typedef, constant, variable, concept?
OK. I will admit that I have got continually confused in the Specification section as to what is a concept, a classs, a typedef etc. Basically I find writing code a lot easier than writing a specification. The specification section needs to be redone to sort all these entities out.
In "dimension": what is "show access to dimension typedef in t1_quantity"???
Ooops. That is an authors note which got left in by mistake. I confess I dont know quite what it means myself, but it shouldnt be there.
I am not sure whether "incoherent_unit" is the best term - perhaps traditional or historical or specialised.
The incoherent_units are called such as they dont fit neatly in the S.I system, needing odd multipliers and so on. I should probably make a more in depth study of the S.I terminology and rework the definitions trying to stick as close as possible to the ones in the S.I.
C.G.S. is not linked.
There doesnt seem to be an authority that maintains it. Its kind of a historical curiosity AFAIK. ______________________________________________
4. There should be "How to..." for most importants use of the library. Acknowledgements should be the last page. Not many people will have stamina to read through all the stuff to find out what is the library good for.
Ok. I was taught that Acknowledgements should appear after the intro. However I later found that every institution puts it somewhere different. Again I believe that moving the docs toward a 'star' rather than sequential layout would solve this.
_______________________________________________ 5. A table listing all namespaces and all relevant typedefs inside, all hyperlinked, is missing. The information may be present but not in single location and compact form.
Instead of lot of free text I would much more prefere tables and examples.
OK. This is basically due to a heavy learning curve in learning how to write technical documentation. I will certainly study other docs to see how it should be done.
_______________________________________________ 6. rationale.html: instead of headings as "clarity" a technical description should be used, e.g. "replacing untyped calculation with typed" or so.
Dtto the "More enjoyable programming" and so on.
The example in "More enjoyable programming" doesn't say explicitly what useful happens inside. Even after several readings I am not able to understand what one should expect to learn here.
"Code integration" section is empty of information. And what gets integrated where?
I guess that this is not very meaningful. What I am getting at is that the library can potentially provide a common format for sharing physical quantity data between applications. In practise that aspect of the library hasnt been achieved so far, except for common output format.
_______________________________________________ 7. Not only examples that show how somethink works but also examples that show what fails and what kind of mistakes the library prevents should be included. Such examples could easily get the most useful resource for the learning process.
Yes. I need to add some examples of applications where the library wont help as much as one might think, as well as applications where it works well.
_______________________________________________ 8. All examples in libs/pqs/examples should be linked from the main documentation with explicit note what one should notice in each of them.
Right. That probably means providing a better targetted set of examples too.
_______________________________________________ 9. library_interface.html:
The text like "...the complexities are hidden behind a simple interface and its not necessary to know the full details of the type to make good use of it" >> /dev/null
Well. Unfortunately I have already presented the reader with the complexities in the definition of terms section. I probably need to move that to the end so that the reader can get to the Getting started section.
Path names as #include <boost/pqs/t1_quantity/types/length.hpp>
- I am against showing history related details (t1, t2, ..) to the user. One wants to used the library w/o hassle, not to deal with "t1".
I dont follow what you are saying here. Do you mean that I should use more meaningful variable names?
"Stream output" section - the exmple should mention what is actual output as a comment next to the << operation. All examples should be consistent in style.
OK. It is there but not part of the code block. I will experiment with the layout to see how to do it consistently.
I am not sure whether stream I/O formatting really belongs to the library - there could be large issues with I18N. There should be separate page in the documentation detailing current use and what one could do to use own formatting.
In practise the great advantage of the I/O formatting is that it is very helpful for debugging and examples. Maybe though it should be constructed similarly to standard locales.
Examples: I would like to see longer names for variables than "t", "t1", "m1", "v1". Especially the "t1" irks me in he context.
Ok. I should spend time on more meaningful names.
The terms as "dimensionful" in sectin names should be hyperlinked. Dimension-full feels more readable.
Yes. The hyper-linking needs to be more consistent throughout the docs.
"Addition and subtraction" section: the heavy hyperlinking over and over makes the text practically unreadable. I really recommend to get rid as much of free text as possible and replace it with more structured content (including examples).
This is difficult because the semantics are very complicated. I have tried to start with just demonstrating what can be done in the Getting Started section, followed by more detail but still informal in the 'Semantics of operations section, followed by a rigid spec in the specification section. Maybe I should get rid of one section, but the informal semantics section is much lighter reading than the spec.
"Multiplication, division" - small examples should follow every sentence. It is hard to identify them in the large blob bellow.
"Calculations - performance" - last paragraph is about what?
Its attempting to reassure the reader that the code produced is efficient, as its not clear from the previous sections This should probably be a whole separate section comparing performance with builtin types to see how the two compare in speed code size etc.
_______________________________________________ 10. t1_quantity.html:
The macros - are the really needed when Boost has existing typedefs?
Unfortunately the BOOST_PQS_INT32 macro is needed because it is used as a non-type template parameter. typedefs wont be accepted. BOOST_PQS_REAL_TYPE might be better as typedef.
"Values" section - I am unable to understand what it describes and what it could be used for.
OK. There is some duplication with the MACROS section, as well as some unnecessary customistation which should probably be removed.
I completely gave up reading the rest of page - I do not see structure or some anchor point.
Fair enough. The whole technical section of the docs is a mess and needs rewriting.
_______________________________________________ 11. rest of documentation:
length.html: point (prn) - I guess this means printing (or better prepress) but does everyone know?
OK. Ideally that should probably be a hyper link to a short note about the unit. There are a large number of units. Currently I am using a database. I was vaguely hoping to convert this into an XML database. Adding notes per units could be opart of that work.
The "acre" here is length or breadth?
hmm. That should be an area. I'm not sure how that got there. There is an area::acre too. Looks like a problem in my header generator.
voltage.html: abvolt and such curiosities should be linked to external definition, if only to verify that the implementation is correct.
Perhaps a link to http://www.unc.edu/~rowlett/units/ would be the best.
Yes that might work very nicely.
physical_constants.html: year or so ago library by Paul Bristow providing many constants was on review (and rejected for the time). If it is still worked on it would be good to coordinate the content.
The same idea was peresented by Alf P. Steinbach. message 5 in this thread on comp.std.c++: http://tinyurl.com/rspvv
extending.html: this should have form of tutorial, with steps 1., 2., 3. ... and description how to verify the result.
reference.html: why is Google redirect there? The CUJ link should be changed to current DDJ.
OK.
_______________________________________________ 12. Normal headers and "out" headers. Perhaps these should be merged together. The code in pqs library uses so much of other Boost functionality that additional <iostream> and <sstream> won't make difference on compilation time (it takes seconds to tens of seconds to compile any example on my machine with Intel 9, the streams alone take less than second).
IMHO it is worth to make the lib simpler to use.
OK. That may be a good idea.
_______________________________________________ 13. t1_quantity/operations/compare.hpp:
The #include <cmath>
should be replaced with
#include <boost/compatibility/cpp_c_headers/cmath>
which works for Intel C++ plugged in VC6 IDE with old Dinkumware.
OK.
The header <boost/compatibility/cpp_c_headers/cmath> has a bug (abs() is missing) but it has been reported.
OK. I'll try that out. _______________________________________________
14. typeof_register.hpp: the warning against Typeof changes after admitting to Boost should be updated/removed.
OK. Hopefully Typeof will be out soon :-)
_______________________________________________ 15. quantity_traits.hpp: the constants max_default_named_quantity_tag would look better as MaxDefaultNamedQuantityTag. It is rather conventional style.
OK. I may remove this anyway, as its rather obscure.
--------- The names in template<typename T, typename S> could be more expressive.
Again I think that I may remove converter customisation from the interface. I dont see a great advantage in having it there.
_______________________________________________ 16. Indentation style. The classes would be better moved to the very left side (i.e. ignoring namespaces). Currently way too many lines get splitted in half and the result is harder to parse (by humans).
4 or 8 characters could be gained for lot of the code.
OK. I will try it, though I prefer indenting consistently as a rule
_______________________________________________ 17. config.hpp: the BOOST_PQS_SUPPRESS_VC8_ADL_BUG should be likely limited to VC8.
OK.
-------------- BOOST_PQS_DEFINE_PHYSICAL_CONSTANTS_IN_HEADERS may have some comment nearby what to do when compiler doesn't support the feature.
OK.
_______________________________________________ 18. concept_checking.hpp: if possible style of #include should be the same all over:
#include "boost/type_traits/is_arithmetic.hpp" #include <boost/concept_check.hpp>
OK.
------ Quite a large block of commented out code exists here. It should be used or removed.
This is a function of moving from concept checking classes to enable if and other forms. I experimented with various compiletime error diagnostic schemes and havent really decided which is best.
------
Perhaps BOOST_STATIC_ASSERT could be used instead of rolling out own Assert (just a minor point).
I found that useful for derivation: struct myClass : Assert<Pred>{/*...*/) gives a nice error message including a local pqs concept-checking header which helps to identify it. Again it is a question of deciding on one style I guess.
The Pair<> could be possibly replaced by std::pair.
Thats true, or mpl::pair.
_______________________________________________ 19. utility/timer.hpp: this looks as if it should be in test/ or examples/ folder.
Yes. There is some overlap with other boost timers too!
_______________________________________________ 20. The whole pqs/two_d: is this documented somewhere or is it start of future work? Dtto three_d.
Not documented yet. These are all intended to be supporting framework for the library. Its potentially a huge library.
_______________________________________________ 21. t1_quantity/t1_quantity.hpp:
public function get_united_value() returns something from detail sub-namespace.
That could probably be shut.
_______________________________________________ 22. t1_quantity/types/capacitance.hpp (and others):
Types as GF (giga Farad) or even much bigger are defined. I am not sure whether in current universe such capacitance could exist. Perhaps those physically impossible types could be pruned out to avoid mistakes caused by typos (for example mF and kF are easy to mis-type).
When someone wants GF then he may add such thing manually.
Or perhaps there could be a macro like USE_FULL_EXTENT_FOR_UNITS that enables such types.
Yes . especially bad for Temperature. These headers are generated. I should add some limits per header.
-------- And when I am at it: when one uses nF and mF mistake is easy to happen and hard to spot.
I would love to have typedefs with names like nano_farad and micro_farad available as well. The cost of typing is smaller than searching for a bug caused by wrong unit.
Thats worth thinking about, but I would be concerned about the duplication. I prefer one interface typedef. I'm not sure about that.
_______________________________________________ 23. The t1_quantity/types/xxx.hpp should contain exact information where in the documentation look for recipe how to create new types.
(And the docs should refer this file.)
Yes. That needs a lot more work.
_______________________________________________ 24. t1_quantity/types/time.hpp:
I would prefer to spell a type as "week" instead of "wk".
The names are based on those in the SI. That seems to be a good reference for the names of units, and as the library matures I am more and nmore convinced that it should closely follow the SI specs if possible.
_______________________________________________ 25. Naming: people may not like long names like boost:pqs:xyz.
Perhaps there could be a macro that, given a namespace (i.e. global) and common name prefix, will create all typedefs where the user likes them. Something as
BOOST_UNITS_DECLARE_ALL_IN(::, unit_)
and then "unit_m" would be meters.
Maybe. but user can use namespace alias , typedef or using for preferred set of units. Unless the current mechanism is unacceptable I am in favour of not providing alternatives.
_______________________________________________ 26. The library provides dimensionless units (if I understanding it correctly).
Its not intended too. Its intended that units are only applied to dimension-ful quantities, (except for angles)
For many applications these would be the most useful ones. There should be page dedicated only to this functionality, not bothering a read with Farads and inches.
As it stands this is part of the implementation in <boost/pqs/detail/united_value/> . It may be worth making it a public part of the library, though I hadnt thought about that before.
There should be examples like number_of_apples = number_of_oranges; // error number_of_fruit = number_of_apples; // OK that give enough of clue without need to read the whole documentation.
As it stands the library is concerned with the SI definition of physical quantities. What you describe is a different library AFAICS. (Often requested by boosters). PQS isnt that one though.
_______________________________________________ 27. The documentation should say (perhaps I missed it) whether the different unites could be used for overloading of functions, with example(s).
OK.
_______________________________________________ 28. As I read the code, Boost.Serialisation is not explicitly supported and won't work by out of box. I guess it should be reatively easy to add.
Yes. I did a demo with boost Serialisation. I should add it to examples. Thanks again for your comprehensive review. regards Andy Little