|
|
Boost : |
From: Andy Little (andy_at_[hidden])
Date: 2006-06-03 19:38:54
"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++:
> 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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk