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

From: David Abrahams (dave_at_[hidden])
Date: 2006-06-07 07:59:08

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

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.

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.

Also is AbstractQuantity::dimension a type or a value?  It could be
either.

+--------------------------------------------+------------------------------------------------+
|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?"

+--------------------------------------------+------------------------------------------------+
|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?

3. Why isn't DimensionallyEquivalent a conforming metafunction?  It
   costs essentially nothing.  

+--------------------------------------------+------------------------------------------------+
|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?

Some of the table cells are split across pages. (e.g. p. 30-31)

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.

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?

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.

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com