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

Subject: Re: [boost] [Review:Contract] Late review
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2012-09-02 19:15:17

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.  First of all, the fact
that a member is not public doesn't necessarily
imply that it doesn't require the invariants to hold.
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.

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

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

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

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

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

In Christ,
Steven Watanabe