Re: [boost] [Review.Contract] Vicente's review

Subject: Re: [boost] [Review.Contract] Vicente's review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2012-09-02 18:14:34

On Sun, Sep 2, 2012 at 2:39 PM, Vicente J. Botet Escriba
<vicente.botet_at_[hidden]> wrote:
> Hi,
>
> here is my short review.
>
> My vote is YES subject to these condition.

Thanks a lot for the review.

> * Removal of the Concepts part.
> The use of Boost.ConceptCheck to emulate C++ concepts proposal is in my
> opinion erroneous. Boost.Concept.Check is used to assert the actual
> parameter satisfy statically the concept conditions, while if I'm not wrong
> the C++ Concepts proposal use the requires clause to state the conditions of
> a possible instantiations. Using the same grammar with different semantics
> is confusing. In a last resort, if this feature is preserved, the requires
> keyword must be changed by another one.

If that were to be the final decision, I'll remove the interface to
Boost.ConceptCheck or I can use any (alphanumeric) mane rather than
requires (e.g., where).

However, I can use the requires clausule to do multiple things:
1. Check concepts defined using Boost.ConceptCheck (current, I'd leave
it as is).
2. Check static_assert (under consideration, to be moved here from
within pre/post/inv).
3. Check Boost.Generic-like concepts implemented using C++11
expression SFINAE (for future... this will be *a lot* of work...).
I should be able to support of all those because I can distinguish 2
using the pp (starts with static_assert) and I can distinguish between
1 and 2 using the compiler for example using a (hidden) tagging base
class to all concepts defined for 3.

IMO, I'd stick with requires, leave 1 as is, maybe add 2 for now, and
consider 3 for future.

> * static assertions are removed from the pre/post conditions. They can be
> moved to a specific "static assertions" section at the same level than the
> requires clause. This section could also include the concept check included
> now in the requires clause.

OK (but I'd personally bundle all into requires).

> * The library extends the EDSL to use directly C++11 features, in particular
> move semantics.
> Now that since the C++11 standard is released, more and more people are
> using C++11 specific features. I, in particular, will not use Contract if I
> can not make use of C++11 features provided by the compiler.

I'd prefer to add C++11 in future releases because the lib is usable
as is for C++03 and adding the C++11 features will take time but it
won't break the interface. Of course, I'll comply with whatever the
final decision is.

> * The contract broken handlers are noexcept, so that adding contracts don't
> change the original prototype.

They will be when C++11 support is added (again, I'd prefer later but
I'm OK otherwise even if that will delay a possible release).

> === STANDARD REVIEW QUESTIONS ===
>
> * What is your evaluation of the design?
>
> While Lorenzo has show that using the preprocessor we can define a EDSL that
> is close to the native syntax (besides the parentheses stuff), the resulting
> interface is *closed* and interacts badly with c++ features not taken in
> account by the EDSL. I'm guessing that the user can not extend this EDSL
> without modifying the library.

Correct.

> This is also the case of other libraries that
> make use of the pre-procesor to define a EDSL as Boost.Parameters or Toward
> Boost.Generics.
>
> It will be great if the user can extend the EDSL, but I suppose this could
> be very hard, if not impossible.

I'm not even sure where to start to implement something like that...
the lib is already complex the way it is... let alone if it supported
some sort of IDL... I guess I'll start by thinking about it in my
casual time now that you have raised the question :)

> I understand that Lorenzo is trying to reduce the preprocessor time using
> whatever is needed, but making the interface more complex would not help to
> improve the usability. I'm not sure the _TPL macros are really needed and
> whether the resulting code could perform better or not.

To give you a definitive answer, I should really provide compile-times
with and without _TPL. That's not trivial but I'll add a ticket to try
to do that at some point. On the positive side, having the _TPL now
and removing them later will not break backward compatibility.

> * What is your evaluation of the implementation?
>
> I've not look inside.
>
> * What is your evaluation of the documentation?
> Very good.
>
> I would appreciate if the documentation includes a sections showing what is
> behind the scenes, what is generated by these macros.
>
>
> It would be great if you add a section/table with all the limitations, as
> e.g. these ones
>           * Function and array types cannot be directly used as function
>     parameter types within the contract macros ...

This will be removed (at the cost of more complex syntax than the
typedef but sometimes needed for deduced function template
parameters).

>         * Each function parameter must always specify both its type and its
>     name (parameter names can instead by omitted in usual C++
>     declarations).

This will be removed.

>     * All tokens must be specified in the fixed order listed in theGrammar
> section (it is not possible to specify
>     postconditions before preconditions,|volatile|  before|const|
>     for member functions, etc).
>
>     * Unfortunately, this library does not allow to specify contracts for
>     unions.
>
>     * Unfortunately, when member initializers are specified, the
>     constructor body must be defined together with its declaration and
>     contract

Will do.

> * What is your evaluation of the potential usefulness of the library?
>
> I don't think the library could be used in a industrial context.
>
> The domain of this preprocessor EDSL is too large (C++ declarations +

Yep, even if you just leave at that "C++ declarations" the domain is
huge (likely it doesn't trickle into the definitions...).

> contracts + named parameters + concepts) and we will need to expand it yet
> to be useful (c++11, compiler specifics, ...). While the syntax is quite
> close to C++ syntax the library intends to emulate, the compiler errors will
> be quite unreadables.
>
>
> * Did you try to use the library? With what compiler? Did you have any
> problems?
>
> No.
>  * How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
> I have not do a in-depth study. I has been concentrated inspecting the
> usability of the library and trying to identify the limitations it could
> introduce.
>
> * Are you knowledgeable about the problem domain?
>
> Yes, from a theoretical point of view. I have never used contract
> programming in real world projects.

Thanks :)
--Lorenzo