Review: concept checks.

From: John Maddock (John_Maddock_at_[hidden])
Date: 2000-11-20 07:51:04

First let me say that I like the idea of a concept checking library, and I
think that Jeremy's submission should be accepted into boost.

I've only skimmed through the docs, but some things I noticed:

Under "Creating Concept Checking Classes" you use "prefix" several times
when you mean "suffix".

In the same section you say:

"One potential pitfall in designing concept checking classes is using more
expressions in the constraint function than necessary. For example, it is
easy to accidentally use the default constructor to create the objects that
will be needed in the expressions (and not all concepts require a default
constructor). This is the reason we write the constraint function as a
member function of a class. The objects involved in the expressions are
declared as data members of the class. Since objects of the constraints
class template are never instantiated, the default constructor for the
concept checking class is never instantiated. Hence the data member's
default constructors are never instantiated (C++ Standard Section 14.7.1
9). "

However VC6 will instantiate all member functions of a template at the time
that the template is instantiated - obviously this is non standard
behaviour - and I don't know how this interacts with implicit default
constructors, but it may be worth looking into (if necessary one workaround
would be to add a template constructor - this would prevent the default
constructor from being auto-generated without being instantiated itself).

Under "Programing With Concepts", you rightly indicate that concepts should
be minimal, and then go on the define LessThanComparable as having all the
comparison operators.  I don't agree with this at all.  The defining
document is the standard, and that requires that the LessThanComparable
concept implement operator< and nothing else.  The example you give is
std::stable_sort, but again the standard makes it clear that this requires
only operator< _and nothing else_, not even an operator==.

Under "Implementation",
You have used a do{}while loop to scope your dummy variable, is this
actually required?  Could you not just use:

#define REQUIRE(__type_var, __concept) \
{ \
  void (__concept##_concept <__type_var>::*__x)() = \
    BOOST_FPTR __concept##_concept <__type_var>::constraints; \
 (void) __x; }

Which may (should?) generate less code in some cases.

Implementation
~~~~~~~~~~

On the whole this looks fine: in an ideal world I would have liked an
implementation that generated neither code nor data, but this appears not
to be possible.  (Footnote: I have played around with an alternative that
used the sizeof operator to convert expressions to
integral-constant-expressions.  With this it's possible to implement the
REQUIRES macro as a typedef, that can be used at namespace/function or
class scope.  Unfortunately I found problems with concept-checking code
that should have failed being compiled silently).  Given that your
implementation does generate code, I think I would like to see concept
checking turned off when NDEBUG is defined.  I appreciate that there are
some compilers that elliminate unused code at link time, but equally there
are many others that do not do so (Borland's compiler for one).

I notice that boost/pending has both concept_archetypes.hpp and
concept_skeletons.hpp, these seem to do the same thing - or am I missing
something?

Test Code
~~~~~~~

None of the test programmes build with Borland C++, on further examination
this appears to be due to a non-standard standard library from Rogue Wave. 
I also note that you have added #if's to the code to allow the test
programs to build with other compilers that ship with non-standard
libraries, the MSVC one seems to be particularly troubling as it just
comments out the whole of stl_concept_checks.cpp reducing it to:

int main() { return 0; }

IMO this is deeply misleading, if a library is non-standard in any way then
this file _should not compile_.

I think I would like to see the test programs split slightly:

concept_checks_test.cpp : new file feeds archetypes into concept checks,
just checks that the compiler can cope with concept checking code.
class_concept_checks_test.cpp : new file feeds archetypes into concept
checks used at class scope, just checks that the compiler can cope with
concept checking code.

stl_concept_checks.cpp: validates standard library types.
stl_concept_covering.cpp: validates standard algorithms.

The first of these three should pretty well always compile, the second if
the compiler supports function-pointer non-template params, and the last
two, only if the standard library is fully conformant.

Future Directions
~~~~~~~~~~~

I like the concept coverage classes a lot, however what I would really like
is a set of wrappers that perform both compile time checking and run time
checking.  For example an input iterator has to be used in a particular way
- with alternating calls to operator++ and operator*, and obviously we
can't go past the end of the sequence.  These are the kind of things that
can only be checked at run time, I appreciate that the SGI STL has a debug
mode that incorporates this kind of checking, but not all of us routinely
use that library (and sometimes we write our own iterators and algorithms
rather than using the standard supplied ones).  This is not a criticism of
the concept checking library, really it's a request for a whole new library
(!) :->

Anyway, I hope that this provides some food for thought, keep up the good
work!

- John.