From: Andy Little (andy_at_[hidden])
Date: 2006-06-07 09:21:31
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