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: Robert Kawulak (robert.kawulak_at_[hidden])
Date: 2008-12-04 19:10:04

Hi Thorsten,

> From: Thorsten Ottosen

> 1. I don't like that the exeption object broken_contrait takes an 
> std::string argument since that may throw std::bad_alloc. I'm 
> fine with that in my own code, but I think libraries should try to 
> avoid it. See also
> 
> http://www.boost.org/community/error_handling.html

It does so because its base class (std::logic_error) requires this (it doesn't
have a default constructor). I think this is fine for the default case, while
you may change the type of exceptions thrown very easily should the possibility
of throwing std::bad_alloc be a serious issue.

> 2. I prefer functions like
> 
>    change_constraint(c, is_positive());
> 
> to be members. We had similar discussion for some function in 
> the Bimap 
> review. Basically, it is nicer for the user to press . to get 
> a list of 
> functions than to search the boost documentation for the 
> right function.
> 
> This comment applies to all the free-standing functions.

Better editors' support is indeed nice, but on the other hand it is advised[*]
to prefer minimal and simple interface of a class, and I strongly agree with
this. If something can be done without the need to access the private data
directly, then it should't be implemented as a member function. This improves
encapsulation and helps to maintain order in the code.

[*] Herb Sutter, Andrei Alexandrescu: C++ Coding Standards, item 44. Prefer
writing nonmember nonfriend functions.

> 3. In this code:
> 
> ignoring_even_int j(1); // exception! (cannot ignore 
> construction with 
> invalid value)
> 
> is there no way to get an assertion instead of the exception?

Sure there is, simply provide an error policy similar to the one used in the
example, but with assertion instead of throw. Also, if you provide error policy
with empty invocation operator, constrained class would fire an assertion by
itself (it verifies every result of error policy invocation). I made an example
with exception rather than assertion consciously, because it's better (safer) if
people follow an example that works always, not only in debug mode.

> 4. Is your library powerfull enough to implement something like
> 
> http://biology.nmsu.edu/software/probability/index.html
> 
> ? If not, what would it take to make that possible?

After a brief glance (sorry, I have no time right now for deeper analysis) I
suspect that Constrbained Value library could provide the basic building blocks
for this library, but right now I can't tell how much the designs of those two
libraries are compatible.

> 5. The docs say
> 
> "Constrained objects can be used as a debugging tool to verify that a 
> program operates only on an assumed subset of possible 
> values. However, 
> it is usually desirable to check the assumptions only in 
> debug mode and 
> avoid the checks in release code for performance reasons"
> 
> Have you made any benchmarks? In particular, I would be interested in 
> those for bounded_int or bounded_float. 

Yes, I've compiled some simple examples (with GCC 4.3.2) to assembly code with
optimisation on (-O3) and compared the result to the result of compilation of
the same code, but with unconstrained replaced by the underlying type. The
assembly code was identical.

For example, the code:

        int fun(int i)
        {
            unconstrained<int>::type u(i);
            u++;
            return u;
        }

compared to identical version, but with unconstrained<int>::type replaced by
int.

> (bounded_float/bounded_double,bounded_unsigned etc. are provided, no?)

As to bounded_float/bounded_double, see point 6.
As to bounded_unsigned - no, because bounded_int works with any type values of
which can be used as template arguments (the 'int' stands for 'integral' rather
than 'int' alone). So you don't need another type, just use
bounded_int<unsigned, ...>.

> 6. My main use case would be to use constrained floating 
> point values, 
> but the docs state:
> 
> "Why C++'s floating point types shouldn't be used with 
> bounded objects?
> [...] there exist a number of myths of what IEEE-compliance really 
> entails from the point of view of program semantics. We shall discuss 
> the following myths, among others: [...] "If x < 1 tests true at one 
> point, then x < 1 stays true later if I never modify x."
> 
> I don't fully understand this...AFAIK, this is not allowed 
> for an IEEE 
> complient implementation. How could we ever implement *anything* with 
> respect to floats then?

I also don't like this, but this is the reality (see
http://www.parashift.com/c++-faq-lite/newbie.html#faq-29.18 too). It's very
important to realise that floating-point operations may be not repeatable.
Unfortunately, to be able to have a reliable constrained object, the comparison
of two values *must* be repeatable.

I've heard of libraries for "reliable floating-point computations" and of
compiler switches that turn some surprising floating-point optimisations off,
but I still don't feel competent enough in this respect to find a way that
*guarantees* proper working of constrained objects. If somebody does -- you are
welcome to help. ;-)

> Anyway, here's my review vote:
> *************************'****
> 
> I think this library should be accepted into Boost provided 
> that minor 
> changes are done. The docs are nice and clear, and the library gives 
> some quite interesting examples of how powerful the library 
> is. Nice work!
> 
> I do think that the above issues should be thought really hard about, 
> especially 1,2,3 and 6. If to is not possible, I change my vote to no.

Thanks for your time. I hope we can work out a consensus on the items upon which
we don't agree. ;-)

Best regards,
Robert