From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2006-06-03 14:18:50
"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 ) and the usability of the
library looks within reach of normal developer.
1. The name "pqs" should be changed to
something a little bit more descriptive.
"unit" feels quite handy.
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
I would recommend to avoid the t1, t2, ...
nomenclature. I know about few projects
named tX, it is better to avoid such associations.
The funny "physical_quantity" uses underscore
intentionally? Shoudn't italics or different font
What may be implemented in the future should
be moved out of introduction. A novice will
have enough of troubles to grasp what's
Marketing ala "Increases programmer satisfaction
and enhances productivity" should be left out.
It is up to the user to decide usability
of the library.
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.
I was not able to understand the last paragraph
in base_dimension section.
It shlould be always spelled what every term means
in C++: class, typedef, constant, variable, concept?
In "dimension": what is
"show access to dimension typedef in t1_quantity"???
I am not sure whether "incoherent_unit" is
the best term - perhaps traditional or historical
C.G.S. is not linked.
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.
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.
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?
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.
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.
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
Path names as
- 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".
"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.
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
Examples: I would like to see longer names for
variables than "t", "t1", "m1", "v1".
Especially the "t1" irks me in he context.
The terms as "dimensionful" in sectin names
should be hyperlinked. Dimension-full feels
"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).
"Multiplication, division" - small examples should
follow every sentence. It is hard to identify them
in the large blob bellow.
"Calculations - performance" - last paragraph is
The macros - are the really needed when Boost
has existing typedefs?
"Values" section - I am unable to understand
what it describes and what it could be used for.
I completely gave up reading the rest of page
- I do not see structure or some anchor point.
11. rest of documentation:
length.html: point (prn) - I guess this means
printing (or better prepress) but does everyone know?
The "acre" here is length or breadth?
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.
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.
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.
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.
should be replaced with
which works for Intel C++ plugged
in VC6 IDE with old Dinkumware.
has a bug (abs() is missing) but it has
14. typeof_register.hpp: the warning against
Typeof changes after admitting to Boost
should be updated/removed.
15. quantity_traits.hpp: the constants
max_default_named_quantity_tag would look
better as MaxDefaultNamedQuantityTag.
It is rather conventional style.
The names in template<typename T, typename S>
could be more expressive.
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.
17. config.hpp: the BOOST_PQS_SUPPRESS_VC8_ADL_BUG
should be likely limited to VC8.
may have some comment nearby what to do when
compiler doesn't support the feature.
18. concept_checking.hpp: if possible style
of #include should be the same all over:
Quite a large block of commented out code
exists here. It should be used or removed.
Perhaps BOOST_STATIC_ASSERT could be used
instead of rolling out own Assert
(just a minor point).
The Pair<> could be possibly replaced
19. utility/timer.hpp: this looks as if it should
be in test/ or examples/ folder.
20. The whole pqs/two_d: is this documented
somewhere or is it start of future work?
public function get_united_value()
returns something from detail sub-namespace.
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
Or perhaps there could be a macro like
USE_FULL_EXTENT_FOR_UNITS that enables
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.
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.)
I would prefer to spell a type as "week"
instead of "wk".
25. Naming: people may not like long names
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
and then "unit_m" would be meters.
26. The library provides dimensionless
units (if I understanding it correctly).
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.
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.
27. The documentation should say (perhaps I missed
it) whether the different unites could be used
for overloading of functions, with example(s).
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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk