Boost logo

Boost :

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


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk