[boost] [Review:Contract] Very late review

Subject: [boost] [Review:Contract] Very late review
From: John Bytheway (jbytheway+boost_at_[hidden])
Date: 2012-09-14 22:23:57

I was out of touch for most of the review period, but I wanted to submit
a review of Contract anyway.  The review manager is of course welcome to
ignore my comments or otherwise give them short shrift as he sees fit as
a consequence of this delay, but I hope Lorenzo will welcome more
feedback :).

Firstly, I would like to add my voice to those who are congratulating
Lorenzo on the quite remarkable feat this library represents.  It has
obviously required extensive and careful thought and effort to reach
this state.

It is primarily because of this evident conscientiousness that I will
vote YES, I think the library should be accepted into Boost.  I concur
with previous reviewers that it is unlikely to suit industrial use, but
it is an important experiment, and one which will benefit from the
additional exposure granted by inclusion in Boost.

My review follows.

Problems using the library
==========================

I only experimented very briefly, but found one problem (which may well
be a clang bug, but you should probably investigate).

The following program I tried works with gcc-4.7.1 but not with clang
r163674 (both in C++11 mode):

#include <contract.hpp>

CONTRACT_FUNCTION(
    int (postinc) ( (int&) value )
        precondition( value < std::numeric_limits<int>::max() )
) {
    return value++;
}

int main()
{
  int i = std::numeric_limits<int>::max();
  postinc(i);
  return 0;
}

The first few errors with clang all look like:

In file included from contract-failure-test.cpp:1:
In file included from .../contractpp_0_4_1/include/contract.hpp:14:
.../contractpp_0_4_1/include/contract/broken.hpp:359:7: error: call to
object of type 'thread_variable<broken_contract_handler>' is ambiguous
    { aux::broken_handlers<>::precondition(context); }
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.../contractpp_0_4_1/include/contract/aux_/thread_variable.hpp:42:5:
note: conversion candidate of type 'void (*)(const contract::from &)'
    operator T() const { // Sync so must return object copy (by value).
    ^
.../contractpp_0_4_1/include/contract/aux_/thread_variable.hpp:49:5:
note: conversion candidate of type 'void (*)(const contract::from &)'
    operator T() const volatile { // Sync so must return object copy (by
value).
    ^

Minor issues in the docs
========================

(I'm not mentioning issues already raised in another review)

Tutorial
--------

- I think it's slightly unfortunate that your example function name
("postinc") shares a four-character prefix with "postcondition".  This
threw me a couple of times when I was reading too fast.

- "specify both its type and its name (parameter names can instead by
omitted in usual C++ declarations)" -- I found this phrasing confusing.
 I think you were planning to remove this constraint anyway so it
doesn't matter, but FWIW I would recommend something like "specify both
its type and its name (unlike conventional C++ declarations, where
parameter names may by omitted)".

- "and assigning it to the return keyword" -- also confusing.  Perhaps
"and assigning to it from the return keyword"

- "destructors cannot specify neither preconditions nor postconditions"
-- double negative; should be "destructors can specify neither
preconditions nor postconditions"

- The whole "Forward Declarations and Body Definitions" section is a bit
odd.  The code example doesn't seem to match up with the text.  For
example, you say "less and natural::get are defined together with their
declarations", but in fact less is forward-declared at the top of the
file with no definition.

Advanced topics
---------------

- "the postcondition result = n * factorial(n - 1) has factorial
computation complexity" -- this is not true; the factorial function as
defined here has linear complexity, not factorial complexity.  (If it
were using arbitrary precision integers it would be something like O(n^2
log(n)^2) but still certainly polynomial, not super-exponential!)

- "preconditions cannot only be weakened" -- again s/cannot/can/

Named parameters
----------------

- By using "requires" here in the pseudocode example with a different
meaning from the rest of the library you are further muddying the
waters.  If you change the "requires" keyword that is used by the
library to something else (as discussed below) then this will no longer
be a problem.

- In footnote 61 (Boost.Parameter bug) could you please link to the bug
so that future readers will know when it is fixed.

Doxygen
-------

You've mentioned a few times the the Doxygen preprocessor can't cope
with your preprocessor metaprogramming.  Have you submitted bugs to
Doxygen?  Do you know how receptive they are to improving their
preprocessor conformance?  This library might be a compelling incentive
for them to do so.

Design issues
=============

The vast majority of the design is clearly well-grounded in existing
practice and practicality.  Nevertheless, I have a few issues to raise.

Concepts
--------

I agree with the earlier thread that you should not use the term
"requires" in the way you currently are, because it has a different
meaning in the Concepts proposal.  Alternatives as discussed there such
as "check" would be an improvement.

Contract broken message
-----------------------

The above example program generates the following broken contract report:

precondition number 1 "value < std::numeric_limits<int>::max()" failed:
file "contract-failure-test.cpp", line 8

I would like my editor (Vim, though this comment applies equally to
others) to automatically take me to the line where the failure occurred.
 I could of course configure it appropriately, but it might make your
users lives easier if you used a standard error format which editors
will recognize by default, such as:

contract-failure-test.cpp:8:precondition number 1 "value <
std::numeric_limits<int>::max()" failed

Furthermore, I suspect that it is quite likely that these contract
broken messages might occur at odd times, such as when the program is in
the midst of printing something else to stdout/stderr, so it might be a
good idea to prefix all your messages with a newline.

Best of all, perhaps, would be to provide a customization point for the
message format (which doesn't entail changing all five broken handlers).
 This would allow me to make these changes to the messages without
disturbing other users.

Contract broken handlers
------------------------

Based on your example, I presume that if I install a custom handler for
broken contracts, and that handler returns without throwing, then
execution will simply continue merrily without regard to the detected
contract violation.  I can see why you might feel the need to provide
such functionality ... but my first instinct is that if a custom handler
returns the library should take over and call std::terminate anyway
(this is similar to the way that a constructor function-try block will
not allow you to swallow an exception thrown by a base or member
constructor).  But I don't feel strongly about this.

At the least, you should document this turn of events more clearly; I
only figured it out from the very last footnote in advanced topics.

Configuration macros
--------------------

This is my biggest concern with the library at present.  You provide the
various macros for configuration (CONTRACT_CONFIG_NO_PRECONDITIONS etc.
as in <contract/config.hpp>).  However, they are all essentially global
constants.  Suppose I am writing an application that depends on a
library, and they both use contracts in their declarations.  I would
like to have only preconditions active for the library, but all
contracts checked for the application.  There is no way to achieve this.

CONTRACT_CONFIG_DO_NOT_SUBCONTRACT_PRECONDITIONS causes a similar but
different problem.  If the library subcontracts preconditions then I
can't disable them for my application.

I think you need to provide a means to turn the various features on and
off on a per-project basis.

As one suggestion, suppose that you took extra arguments to your macros,
so that the above example became:

CONTRACT_FUNCTION(
    MYAPP_NO_PRECONDITIONS, MYAPP_NO_POSTCONDITIONS,
    int (postinc) ( (int&) value )
        precondition( value < std::numeric_limits<int>::max() )
) {
    return value++;
}

and then I define a wrapper macro for my app which substitutes these
arguments for me, and therefore the example becomes:

MYAPP_CONTRACT_FUNCTION(
    int (postinc) ( (int&) value )
        precondition( value < std::numeric_limits<int>::max() )
) {
    return value++;
}

Or perhaps rather than many variables it would be better to have a
single MYAPP_CONTRACT_CONFIG value which contained all of them (as a
preprocessor tuple, say) which was passed in similarly.

Anyway, I'm sure you can think of something appropriate.

Philosophical issues
====================

Finally, I'd like to go off on a bit of a tangent and talk about a
philosophical problem with the design.  It has been touched on by other
reviewers but I'd like to go into more detail.

Your library design is very syntactically invasive.  As the docs
indicate, this makes it unfamiliar, slow to compile, and potentially
difficult to debug.  But none of these is the worst problem.

To worst problem is that only one library can do this.  You've already
seen this, and done your best to integrate the other Boost libraries
that have similar requirements (Parameter and Concept) but this approach
doesn't scale.  I'm sure at least one of the many reflection libraries
proposed to Boost does something similar.  Will you integrate that too?
 What about non-Boost libraries.

The only scalable solution is to write a framework for customized
definitions.  You've mentioned the possibility of using the knowledge
your alternate syntax has to expose extra data (e.g. function access
level) at preprocessor time.  But that's not enough.  It would have to
allow other library authors to inject code into definitions in the way
Contract or Parameter needs to; even provide whole new definition
syntaxes as Property does.

Given such a framework, each other library (Contract, Parameter, a
reflection library, and whatever else) would be written using the
framework.  It should then be possible to use whichever a user desires,
at the same time, for the same declaration, without those libraries
having any knowledge of each other.

Such a framework would be akin to the language customization features
that some other languages have (and which have also been mentioned in
other threads of this review).

To be clear, I am certainly not demanding, nor even proposing, that you
implement such a framework.  I think it would be extremely difficult --
more trouble than it's worth.  It would also be very dangerous (for all
the same reasons that it is dangerous to be able to customize the
language on the fly).  So long as Contract remains a primarily
experimental library the current design is perfectly adequate.  I just
think that this direction is the logical conclusion of your efforts, and
wanted to bring it to your attention (though you've probably already had
similar thoughts).

Standard review questions
=========================

  * What is your evaluation of the design?

Barring issues mentioned above, it is an excellent attempt to solve a
very difficult problem.

  * What is your evaluation of the implementation?

I did not examine it.

  * What is your evaluation of the documentation?

I did not read the reference; the rest was very good, with minor nits as
above.

  * What is your evaluation of the potential usefulness of the library?

It has significant potential for use in small projects, to spread the
idea of contract programming, serve as a proof of concept and influence
future standardisation efforts.  It is unlikely to see use in large
scale industrial programming.

  * Did you try to use the library? With what compiler? Did you have any
problems?

Very briefly, with compilers and problems mentioned above.

  * How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I spent three evenings reading the docs and writing my review, and have
been following the development of the library on the list since its
first appearance.  I would have liked to do more experiments, but my
review was already late enough!

  * Are you knowledgeable about the problem domain?

Not to any notable degree.

Congratulations again to Lorenzo on achieving something I would have
thought impossible; with luck it might inspire me to actually try
contract programming.

John Bytheway