Re: [boost] [review][constrained_value] Review of Constrained Value Library begins today

Subject: Re: [boost] [review][constrained_value] Review of Constrained Value Library begins today
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-12-09 19:23:23

----- Original Message ----- 
From: "Jeff Garland" <jeff_at_[hidden]>
To: <boost_at_[hidden]>; <boost-users_at_[hidden]>
Sent: Monday, December 01, 2008 1:30 PM
Subject: [boost] [review][constrained_value] Review of Constrained Value Library begins today

Hi,
Apologies for the late review

- What is your evaluation of the design?
It's a simple library with a simple and elegant implementation of a simple concept.  However, I have some improvements suggestions on the design:

* I find that the extreme example "bounded object with memory" is not a good example of constrained_value because not constrained at all. It is the example of what can be done with the current interface but that shouldn't be able to be done with a constrained class, i.e. define an unconstrained type. I don't understand why an error policy can modify the value or the constraint. I would prefer an error policy taking only two in-parameters (by value or const&) the new value and the constraint. 

* The wrapping and cliping classes are not exactly constrained values, they are adapting a value of a type to a constrained value. So I would prefer to have a different class for them, e.g. constrain_adaptor, taking a constraint adaptor that would take the value by reference and adapt it to the constraint. I don't think constrain_adaptor will need an error policy.

* I don't know if you will add monitored values to your library. IMO it will be better to have them in a separated class. We can monitor values that are constrained or not. "bounded object with memory" could be better considerd as a monitored value.

* The class bounded_int should take another name as e.g. static_bounded.

* I dont like the free functions to change the constraints as change_lower_bound. I think that it is preferable to have specific classes and no typedefs. Classes as bounded can define the specific operations for changing the bounds. Even if there will be a lot of repeated code, what is more important is the user interface.

* The operators have not always a sens for the application. For example, 
    constrained<int, is_even> x;
    ++x;

I would prefer to have a compile error in this particular case instead of an exception. It will be quite interesting to show how a user can catch this error at compile time and avoid this runtime error. 

Maybe we need a parameter to state that arithmetic operators are available.

template<typename V , typename C , typename E > 
constrained< V, C, E , with_arithmetic_operators> & operator++ (constrained< V, C, E,with_arithmetic_operators> &c)

the bounded hierarchy should have with_arithmetic_operators.

* constrained must be default constructible. The library should provide a mean to specify a default value. E. g.
    constrained<default<int, 1>, is_odd >

* The default value for excluding the bound is false, which mean that the bound is included. There is no example of excluding bounds. Here is one

typedef bounded_int<int, 0, 100, throw_exception<>, true, true>::type t;

I think that it will more clear if instead of stating exclusion we state inclusion, and the default be true, 

So we can write 
    typedef static_bounded<int, 0, 100, throw_exception<>, false, true>::type t;

Even in this case the bool values are not enough declarative. It would even be beter to define two templates, open, close (see below)
    typedef static_bounded<int, open<0>, close<100> >::type t;

* I would like the library manage constraints between several variables, e.g the range of valid days values depends on the month. We can need to set/get each varaible independently or all at once. The concept of tuple seems interesting.

typedef static_bounded<int,1,12> month;
typedef constrained<int, between_1_31> day;

struct adapt_day_to _month_constraint {
    bool operator(day& d,  month& m) {
        switch (m) {
            case 1: 
            case 3: 
            case 5: 
            case 8: 
            case 10: 
            case 12: 
                d.change_constraint(between_1_31);
                break;
            case 2: 
                d.change_constraint(between_1_28);
                break;
            default: 
                d.change_constraint(between_1_30);
        }
    }
};

typedef constrained_tuple<day, month, adapt_day_to _month_constraint > date;

date d; 
assert(d.get<day>==1);
assert(d.get<month>==1);

d.get<month>=2;
d.get<day>=30; // exception

The current interface could also be used as

    typedef constrained<tuple<day, month>,  adapt_day_to _month_constraint > date;

but we can not assign the value independently. Of course the user can implement hisself the constrained_tuple class. If I have time I will try to implemnet it.

* The number of parameters of the bounded class is already to high. 

template <
    typename ValueType,
    typename LowerType = ValueType,
    typename UpperType = LowerType,
    typename ErrorPolicy = throw_exception<>,
    typename LowerExclType = boost::mpl::false_,
    typename UpperExclType = LowerExclType,
    typename CompareType = std::less<ValueType> >
struct bounded;

One possibility could be to group the bound related parameters in a bound traits class:
    template <typename ValueType, bool Closed=true>
    struct bound_traits {
        typedef  ValueType value_type;
        typedef  boost::mpl::bool_<Closed> incl_type;
    };

We can define the open/close helper classes as follows:
    template <typename ValueType=undefined>
    struct open: bound_traits<ValueType, false> {};
    template <typename ValueType>
    struct close : bound_traits<ValueType, true> {};

So the class bounded could pass from 7 to 5 parameters.
template <
    typename ValueType,
    typename LowerTraits = close<ValueType>,
    typename UpperTraits = LowerTraits,
    typename ErrorPolicy = throw_exception<>,
    typename CompareType = std::less<ValueType> >
struct bounded;

and used as follows
    typedef bounded< int, open<>, close<> >::type t;
which should be equivalent to afterr managing the undefined type
    typedef bounded< int, open<int>, close<int> >::type t;

The same could be applied to bounded_int
template <
    typename ValueType,
    ValueType LowerBound,
    ValueType UpperBound,
    typename ErrorPolicy = throw_exception<>,
    bool LowerExcl = false,
    bool UpperExcl = LowerExcl,
    typename CompareType = std::less<ValueType>
    >
struct bounded_int;

I would want to be able to write
typedef static_bounded<int, open_c<0>, close_c<100> >::type t;

template <typename ValueType, ValueType Bound, bool Closed>
struct static_bound_traits {
    typedef  boost::mpl::integral_c<ValueType, Bound> value_type;
    typedef  boost::mpl::bool_<Closed> incl_type;
};

template <typename ValueType, ValueType Bound>
struct open_c;
template <typename ValueType, ValueType Bound>
struct close_c;

template <int Bound>
struct open_c<int, Bound>: static_bound_traits<int, Bound, false> {}; 
template <int Bound>
struct close_c<int, Bound>: static_bound_traits<int, Bound, true> {}; 

template <
    typename ValueType,
    typename LowerBound,
    typename UpperBound,
    typename ErrorPolicy = throw_exception<>,
    typename CompareType = std::less<ValueType>
    >
struct static_bounded;

typedef static_bounded<char, open_c<char, 0>, close_c<char,100> >::type t;

Boost.Parameters could also be useful.

- What is your evaluation of the implementation?
I was very impresed the way static and dynamic constrained types has been solved. This is a very elegant implementation. 

I would like the library ensures that the size of the constrained class be equal to the underlying type, at least for those that can't change neither the constraint nor the error policy at runtime.

- What is your evaluation of the documentation?
Good enough. 

Adding more examples will be welcome as:
* Application domain classes using (by inheritance and by containement) a constrained class to see what the user needs to do when it needs to add some methods, and don't want some inherited methods.
*Date type would show the change upper bound at runtime, and see how the values of a variable (month) impact the constraints on another variable (day).
*The example of a low static bounded and runtime dynamic bounded could be a good one to show how the constrained class can be specialized.

- What is your evaluation of the potential usefulness of the library?
Constrained values are used by a lot of applications. Boost.ContrainedValues do it in an elegant way. So yes, it will very usefull.

- Did you try to use the library?  With what compiler?  Did you have any problems?
No, I have no taken the time.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
In-depth study of the documentation, implementation and the review posts.

- Are you knowledgeable about the problem domain?
Except for the floating point issues the constrained value domain is quite simple, so yes I consider I know it.

I'm not sure the library should be accepted in his current state. I would like to say that it should be accepted IF some of the following  suggestions are taken in account, as
    * separation between constrained and constraint_adaptor
    * use of classes instead of typedefs and removal of the free functions
    * specialization of the operators with a possible parameter with_arithmetic_operators.
    * adding a default value template parameter to the value type.
    * reduction of the number of parameters of the class bounded (posible use of open<> and close<>)
    * reduction of the number of parameters of the class bounded_int (posible use of open_c<> and close_c<>)

Hopping some of the suggestions could improve your library,
Vicente