Re: [boost] [Review:Contract] Late review

Subject: Re: [boost] [Review:Contract] Late review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2012-09-02 13:16:12

On Sat, Sep 1, 2012 at 2:59 PM, Steven Watanabe <watanabesj_at_[hidden]> 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_0_4_1/doc/html/contractpp.pdf

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_0_4_1/doc/html/contract__/tutorial.html#ftn.contract__.tutorial.constructors.f0

> - 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_0_4_1/doc/html/contract__/contract_programming_overview.html#ftn.contract__.contract_programming_overview.destructor_calls.f0
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