Re: [boost] [Review][PQS] Concept names?

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