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