On Sat, Sep 1, 2012 at 2:59 PM, Steven Watanabe <watanabesj@gmail.com> wrote:

AMDG

First of all, *Yes, Contract should be accepted into Boost*

I've only really looked at part of the documentation so far. Here's my comments from that. I may have more later depending on how much time I have.

Thanks for submitting a review. If you have additional comments at any time, I'd be most interested in hearing them.

index.html:

- "Contract Programming (CP) allows to specify" Use a gerund (specifying) instead of an infinitive. An infinitive here needs a subject.

OK, and OK to all grammar and styling comments below too.

- "... allowing to find bugs ..." Same problem, although it can't be fixed in the same way, since the participle + gerund would sound awkward. It could be handled with a passive infinitive. (allowing bugs to be found)

- "and it was first introduced by the Eiffel programming language " You can leave out the "it." It sounds awkward to me.

- "Eiffel programming language" Using this repeatedly sounds a bit pedantic. You can just say "Eiffel" after the first occurance.

- The side by side code in the example gets a bit wide. This will definitely be a problem if you want the documentation to be generated as a PDF.

The PDF expands the 3 columns of the table with the same width so the 1st column with the line number becomes way too large and the example doesn't fit :( That's why I generated the PDF on A3 instead of A4 page size but still looks ugly, page 3 of: http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_... Maybe someone can suggest me if there's some PDF option to generate a better layout for this table... but I like to keep 3 columns for a side-by-side comparison.

- I don't understand why the library needs to deal with access control. Why can't the user just use public: private: protected: as usual?

The user can use the access specifiers outside the macros: CONTRACT_CLASS( class (x) ) { CONTRACT_CLASS_INVARIANT( void ) protected: typedef int size_type; // access specifier outside macros CONTRACT_FUNCTION( public void (f) ( void ) // access specifier in macros ) {} }; However, the access specifiers also needs to be in the macros because private and protected functions do not check class invariants. Therefore, the macros need to know if the function being contracted is public of not so to expand the invariant checking code or not. See note [26]: http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_...

- CONTRACT_OLDOF kind of sticks out at me. I don't really like the way it looks having it a macro when everything else is keywords that are parsed by the top level macros.

I know but there's no way to parse it as a keyword because it's nested within the assignment statement. It being a macro is part of the mechanism to handle the `=` symbol: [auto|fundamental_type|(type)] variable_name = CONTRACT_OLDOF expression The pp parses this into 3 traits [auto|fundamental_type|(type)], variable_name =, and expression. The type is either auto, known (fundamental type like int, long double, etc), or wrapped within parenthesis so it can be parsed and stripped away from the front. Then the CONTRACT_OLDOF macro essentially expands to )( so it allows me to separate variable_name = from expression (using pp sequences) that otherwise I couldn't separated because they are unknown tokens plus they contain non-alphanumeric symbols (so I can't use concatenation to handle them).

- "...if there is a bug in the function caller for which push_back is called when size is equal to max_size ..." This clause is somewhat confusing.

- "...with the assertion number to uniquely identity..." s/identity/identify/. Also don't split infinitives.

- "This library suffers of two limitations" s/of/from/

getting_started.html:

- "This section explains how to setup..." "set up" should be two words. "setup" as one word is a noun.

- "... failure of the checked condition does not abort the program, instead ..." Run on sentence.

- "The implementation of this library uses..., templates with partial specializations and function pointers (similarly to Boost.Function), ..." I don't understand the connection between partial specialization and function pointers. Why are they grouped together?

No particular connection, I was just trying to say that my lib implementation uses both... among other things that need to be supported by the compiler.

contract_programming_overview.html:

- "It is assumed that programmes" s/programmes/programmers/

- "...to formally program specifications". Don't split the infinitive. "to program specifications formally"

- "Then, this library aims to be ..." "Then" doesn't sound right here. "Then" is usually used for conditions ("if... then") or sequences (e.g. First.... Then...), but there's nothing for it to connect to here.

- "Contract Programming is characterized by the following type of assertion mechanisms." s/type/types/

- "before the body execution." I think it would sound better to use the possesive "body's execution" or "execution of the body". rather than apposition.

- "Preconditions cannot be strengthen, postconditions and class invariants cannot be weaken. " s/strengthen/strengthened/, s/weaken/weakened/

- I'm not sure that I would consider Subcontracting to be an assertion mechanism.

- "Block Invariants | These are logical conditions that programmers except ..." s/except/expect/

- "...while a contract assertions is ..." s/assertions/assertion/

- "This library implement this feature however it should be noted ..." Run on sentence.

- "protect such a global variable from racing conditions " It's called a "race condition."

- "but that will effectively introduce a global lock in the program" This seems like a classic use case for thread specific storage.

I'll look into it: https://sourceforge.net/apps/trac/contractpp/ticket/70

- "Furhtermore," s/Furhtermore/Furthermore/

- "Contracts are part of the source code, they are..." Run on sentence (comma splice)

- "detailed error messages that greatly helps debugging" "helps" needs to agree with "messages."

- "Contract Programming benefits come to the cost of performance " "to" is the wrong preposition. Try "at."

- "The run-time performances are negatively ..." "performance" should be singular. Plural doesn't really make sense for abstract nouns like this.

- "1. The extra processing required to check the assertions. 2. The extra processing required by the additional function calls (additional functions are invoked to check preconditions, postconditions, class invariants, etc)." How is (2) different from (1)?

Because there is a cost in the assertion instruction itself plus the cost of calling the function that checks pre/post/inv: void f_pre ( ... ) { // cost of calling f_pre (2) assert(...); // cost of executing the assertions (1) assert(...); ... } I din't necessarily had to put all pre/post/inv in separate functions (even if that's really the only sensible way to implement this especially when subcontracting comes into play because then you need to access the base pre/post/inv separately from the base function body).

- "checking function arguments using postconditions " I think you mean /pre/conditions.

- "so that these initializations can relay" s/relay/rely/

- "C++ object construction mechanism ..." You need an article. "The C++..."

- "Check the non-static class invariants, but only if the body threw an exception." This could use a rationale. Destructors are generally not supposed to throw at all.

The rationale is [10]: http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_... Of the point above this one but it might make sense to move the rational to the line you are suggesting, I'll do that.

- "Contracts are only responsible to check the " "for checking" instead of "to check."

- "...to only use public members, Eiffel instead ..." Split infinitive, run-on sentence.

- "...are allowed to brake class invariants..." s/brake/break/

- "Arbitrary code in contracts | No, assertions only." This is a bit misleading, since assertions can execute arbitrary code.

I meant you can't use any statement in pre/post/inv like if, while, for, etc. I'll double check what terminology N1962 and Eiffel use and I'll clarify the docs.

tutorial.html:

- "complete guide on this library syntax" s/library/library's/

- result == oldof valule s/valule/value/

- "int const" IIRC, you can't have a const rvalue of a built-in type.

I need to review the lib and the docs for C++11 features... rvalue included.

- "parameter names can instead by omitted " s/by/be/

- params_postinc You didn't update the pre/post conditions when you copied this from the last example.

- "Function and array types cannot be directly used as function parameter types within the contract macros but extra typedef declarations can be used to workaround this limitation " This workaround doesn't work in function templates where the array element or dimension is deduced.

Yep :( Same issue with using BOOST_INDENTITY_TYPE but for that I support commas in types within the syntax. Maybe I should do the same of function pointers and arrays, I should provide a syntax to support them (so you can use them when deduced in templates) but also suggest that a typedef might get the work done with less syntactic burden. https://sourceforge.net/apps/trac/contractpp/ticket/71

- "the maximum number of supported array dimensions is specified by the CONTRACT_CONFIG_ARRAY_DIMENSION_MAX macro" I don't understand the reason for this limitation.

- "...evaluates all contract conditions in constant-correct context" Add an article "a" before "const-correct."

- "the type of value type" Delete the second "type."

- ConstantCopyConstructible You're definitition of CopyConstructible is wrong. A type that only defines T(T&) is not CopyConstructible.

Oops, how did I mess this up?? I even have the right link to the concept definition: http://www.boost.org/doc/libs/1_51_0/libs/utility/CopyConstructible.html I meant and needed only the copy constructor so I'll name this differently... https://sourceforge.net/apps/trac/contractpp/ticket/72

- "However, it is still necessary to ontract" s/ontract/contract/

- unique_identifiers I think it's worth pointing out that the postcondition can't assume that the precondition was true in the face of subcontracting. I did a double take when I saw old_found.

Yes, in a nutshell that's the point of the example. I'll state that up front as you say it here so (if you are clever) you don't have to wait until the end of the example for it to make sense :)

- "Wakened inherited preconditions ..." s/Wakened/Weakened/

oldof.hpp:

The definition of CONCEPT_OLDOF is a bit dangerous. It can't safely be passed as a macro argument.

What do you think of this bit of PP magic:

#define TEST_EXPAND_EXPAND_OLDOF(x) CONTRACT_OLDOF #define TEST_EXPAND_NOTHING(x) oldof #define TEST_EXPAND_I(x) TEST_EXPAND_ ## x #define TEST_EXPAND(x) TEST_EXPAND_I(x) #define oldof TEST_EXPAND(EXPAND_OLDOF(NOTHING(z))) #define EXPAND_OLDOF(expr) expr

oldof -> expands to itself EXPAND_OLDOF(random text oldof more random text) -> expands to random text CONTRACT_OLDOF more random text

Hum, tricky... this works on MSVC but not on GCC :( Plus it's defining oldof and that doesn't follow the macro naming conventions (even if I understand that it's supposed to expand to itself when used outside of the expanding macro...). Also with the current implementation of the lib #define oldof CONTRACT_OLDOF won't work... The docs should clearly say that CONTRACT_OLDOF should only be used within postconditions old variable declarations, after the =. I could have a configuration macro CONTRACT_CONFIG_DEFINE_OLDOF_KEYWORD that will #define oldof so you can use either oldof or CONTRACT_OLDOF when you define this config macro... This way the non-guideline-compliant #define oldof is at least guarded by a config macro...

General:

- Are ellipsis parameters allowed?

I'll look into it... I should be able to support it if anything with a syntax different than "..." maybe "etc" or ",,,". https://sourceforge.net/apps/trac/contractpp/ticket/73

- Just say "parentheses," not "round parentheses." Parentheses without any qualification always means ().

Thanks, --Lorenzo