Re: [boost] [Review:Contract] Very late review

Subject: Re: [boost] [Review:Contract] Very late review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2012-09-15 02:11:23

On Fri, Sep 14, 2012 at 7:23 PM, John Bytheway
<jbytheway+boost_at_[hidden]> wrote:
> 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 :).

Indeed, thanks a lot for your review!

> 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).
>     ^

Sorry, I only tested on MSVC and gcc. I will fix this issue with clang
(this should be fixable no problem, the only real compiler
comparability issues should be with compilers that have a preprocessor
that cannot handle the macros, the rest should be fixable).

> 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.

I'll try to rename it.

> - "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)".

OK.

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

OK.

> - "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.

I'll try to fix the text.

> 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!)

Oops, I'll fix this.

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

OK.

> 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.

Yep.

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

OK (I'll have to submit the bug first ;) ).

> 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.

I'll submit a but to Doxygen and refer it.

> 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.

Yes, I will change to "check" for checking both Boost.ConceptCheck's
concepts and static assertions.

> 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

OK.

> 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.

I'll consider this.

> 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.

I'll document it and leave it as is (because I think if the user want
to write broken handlers that ignore the failure, he/she should be
able to do so--but at their own risk).

> 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.

I don't have a definitive answer for this but I think it's a good
point. AFAIK, N1962, Eiffel, D, etc don't address this. However, I was
thinking to provide a version of the STL with contracts:
http://sourceforge.net/apps/trac/contractpp/ticket/47

That would be a good example where you might trust the STL
implementation well so not to check post/inv but only pre for the
contracted STL. However, for you code written with Boost.Contract you
want to check all pre/post/inv. Right now that can't be done. Either
you check post/inv for all the code including the STL or for none of
the code including your code.

I saw this issue for the 1st time 2+ years ago however it's still not
obvious to me how to support this but I think the lib should support
this at some point if not in its 1st rev. I will think about this more
so to have at least a backward-compatible plan to support this
feature.

> 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).

I'd agree that is the logical future direction but it might be very
difficult if not impossible to provide for such customization.
However, I didn't really think about this long enough to say anything
sensible about it.

> 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.

Let's hope so... let's hope for contracts, concepts, and maybe even
named parameters for C++1x.

> 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.

Thanks a lot!
--Lorenzo