Hi David, "David Abrahams" wrote
I just took one quick look at the "Definition of Terms" section in the docs and I'm concerned about what appear to be not-quite-concepts being defined and what appear to be the inventive naming conventions used there.
1. General multiword terms such as "physical_quantity_system" should not be spelled with underscores between the words. Nobody else does that; it's confusing because one can't tell whether these are meant to be identifiers
Ok.
2. Many of the terms, e.g. abstract_quantity, seem like they are weak versions of generic programming concepts, and should be strengthened... oh, I see that AbstractQuantity is in fact listed in the the Concepts section of the docs, with an appropriate naming convention. But then, why the redundant terms? Or are they not redundant? Regardless, many of the other terms, such as abstract_quantity_id, anonymous_quantity, etc. seem to be concept-like and don't appear in the concepts section. Are you sure they're not needed in order to rigorously define the library's requirements?
The Concepts section is not ready for prime-time. For example, let's look at the AbstractQuantity table:
+--------------------------------------------+------------------------------------------------+ |Expression |Type | +============================================+================================================+ |AbstractQuantity::dimension |Dimension | +--------------------------------------------+------------------------------------------------+
Okay, so what is AbstractQuantity here? It can't be a type or a namespace, because you just used that for a concept identifier. I'm not kidding, when there's language support for concepts it won't seem so strange that this is confusable. Until then, you should add Boost concept checking classes to the library anyway, and those would occupy the identifiers. There's an established convention for choosing and describing the identifiers in concept tables. Use it. Also, put code elements in a code font.
OK.
And what is Dimension? Your column header implies it's a type, but I bet it's not. And if it is, it violates Boost naming conventions, and must be fixed.
OK.
Also is AbstractQuantity::dimension a type or a value? It could be either.
OK. I guess thats unclear.
+--------------------------------------------+------------------------------------------------+ |AbstractQuantity::id |AbstractQuantityId | +--------------------------------------------+------------------------------------------------+
Same exact problems.
+--------------------------------------------+------------------------------------------------+ |binary_operation<AbstractQuantity |AbstractQuantity D where D::dimension is | |Lhs,Op,AbstractQuantity Lhs>::type |binary_operation<Lhs::dimension, Op, | | |Rhs::dimension>::type and D::id is | | |binary_operation<Lhs::id, Op, | | |Rhs::id>::type, except where Op = pow | +--------------------------------------------+------------------------------------------------+
Okay, left column: this isn't even valid C++ code.
Right column: Everything looks like it could be comprehensible, until you get to "except." Well, what is the requirement in the "except" case? Does "except..." apply to the whole clause or only to the part of the clause after "and?"
Ok I could put brackets around it I guess.
+--------------------------------------------+------------------------------------------------+ |binary_operation<AbstractQuantity Lhs, |[AbstractQuantity D where D::dimension is | |pow, Rational Exp>::type |binary_operation<Lhs::dimension,times,Exp>::type| | |and D::id is the anonymous_quantity_id | +--------------------------------------------+------------------------------------------------+
Left column: not C++.
+--------------------------------------------+------------------------------------------------+ |DimensionallyEquivalent<AbstractQuantity |true if DimensionallyEquivalent<Lhs::dimension, | |Lhs,AbstractQuantity Rhs>::value |Rhs::dimension>::value==true else false | +--------------------------------------------+------------------------------------------------+
Left column:
1. not C++.
2. Your library contains a non-concept-checking template that uses CamelCase? What's wrong with the boost convention? Oh, but I see elsewhere in the docs you do have dimensionally_equivalent. Which is it?
OK. Fair point.
3. Why isn't DimensionallyEquivalent a conforming metafunction? It costs essentially nothing.
Sure its confused...
+--------------------------------------------+------------------------------------------------+ |Dimensionless<AbstractQuantity>::value |bool | +--------------------------------------------+------------------------------------------------+ |IsNamedQuantity<AbstractQuantity>::value |bool | +--------------------------------------------+------------------------------------------------+ |IsAnonymousQuantity<AbstractQuantity>::value|bool | +--------------------------------------------+------------------------------------------------+
Ditto #2 and #3 above. Also, technically speaking it's not clear whether these are types or values. Also, are there _no_ semantics associated with the values? Could I pick true or false for any of them and still have a conforming AbstractQuantity? If so, why bother requiring them?
Yes. These should be functions..
Some of the table cells are split across pages. (e.g. p. 30-31)
Ok.
Value_type should be ValueType. The description of Value_type should not be done in terms of a single template where you intend to require one as a parameter.
Ok.
Why BOOST_PQS_INT32? Doesn't the standard or Boost provide enough types for this without you having to define a macro? And even if not, why not use a typedef instead?
I tried that but it wasnt acceptable as a non-type parameter IIRC.
Aside from nits and naming convention issues, I feel especially strongly that we must not muddle the ideas of generic programming. I think this is an important domain and I hope we'll be able to accept a different version of this library, but, with regret, I vote against the inclusion of this one in its current state.
Ok. Thanks for the review regards Andy Little