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

Subject: Re: [boost] [Review:Contract] Late review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2012-09-03 11:15:32

On Sun, Sep 2, 2012 at 4:15 PM, Steven Watanabe <watanabesj_at_[hidden]> wrote:
> AMDG
>
> On 09/02/2012 10:16 AM, Lorenzo Caminiti wrote:
>> On Sat, Sep 1, 2012 at 2:59 PM, Steven Watanabe <watanabesj_at_[hidden]> wrote:
>>> index.html:
>>>
>>> - 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
>>
>
> I think it would be better to make this explicit
> like noinvariant instead.

This is possible however N1962, Eiffel, D, A++, etc all automatically
disable class invariant checking for non-public member function calls.

> First of all, the fact
> that a member is not public doesn't necessarily
> imply that it doesn't require the invariants to hold.

>From http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n1962.html#class-invariants

...
    The class invariant has the following properties:
    ...
        3. It is called implicitly from public
        ....

> Second, I don't like the idea of making the semantics
> depend on access control.  It goes against the
> way access control works in the core language.
> I think that code written using this library
> needs to be understandable without having to
> read the documentation carefully and features
> that change the behavior in non-obvious ways
> militate against this.

I agree that code written using this library should to be
understandable without having to read the documentation carefully but
/by programmers that understand Contract Programming/.

That only public member functions check class invariants is not
something special about the library. It's a Contract Programming
requirement so users should be familiar with it a-priori (or by
reading the Contract Programming Overview section). It's like knowing
that preconditions are executed before body execution, or that
old-values are copied before body execution, or that subcontracting
checks precondition in logic-or, etc. All of this semantic should be
known to the users, they can learn it from Contract Programming
Overview section, but it's really all about Contract Programming and
there's nothing special about the lib here (the syntax instead for
example is special about the lib).

>>> - 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).
>>
>
> Okay, so you can't parse the = with the preprocessor.
> I think you should really consider getting rid of
> the = entirely then.  You've already replaced '='
> for default parameters with a keyword.  Something
> along the same lines would be more consistent with
> the rest of your grammar.

For default parameters the = is replaced by ", default" and "," plays
a key role. I'd have to replace:

auto old_size = CONTRACT_OLDOF size() // (1)

with something like:

auto old_size, as size() // (2)

I personally prefer (1) because more readable even you are correct
that technically (2) (even better without the "as") would be more
consistent with the rest of the grammar.

>>> - "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.
>>
>
> In that case, I would separate them with a ',' instead
> of "and," so they're separate entries in the outer list.

OK.

>>>
>>> - "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).
>>
>
> You can just fold the function call overhead into
> the cost of checking the assertions, unless you
> want to be really pedantic.  Even then, the
> compiler can eliminate it by inlining.

OK.

>>> - "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.
>>
>
> rvalues aren't a C++11 feature.

Yeah, sorry... I don't know what I was thinking of... I'll fix the int const.
https://sourceforge.net/apps/trac/contractpp/ticket/75

>>> 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 :(
>
> Unfortunately, GCC is correct and there's no way
> to make it work.
>
>> Also with the current implementation of the lib #define oldof
>> CONTRACT_OLDOF won't work...
>
> The only reason is doesn't work is because
> the name oldof is used by the library.
> If I use #define myoldof CONTRACT_OLDOF
> it works fine.

My bad for not investigating the root cause of the errors I was
getting. Then I'll fix this so users that don't like CONTRACT_OLDOF
can just do #define oldof CONTRACT_OLDOF ans use oldof within the
postcondition code :)
https://sourceforge.net/apps/trac/contractpp/ticket/76

Thanks a lot!
--Lorenzo