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: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2008-12-01 12:24:15

> - What is your evaluation of the design?

It seems good.  It's a simple library with a simple and
straightforward design and implementation.  However, I have some
notes:

There seems to be no way to specify compile-time bounds with one bound
unconstrained.  This seems like a useful thing to be able to express.

The docs include some example code on "using constrained objects in
debug mode only", and make reference to using unconstrained<> to allow
use of the .value() member function.  Why not replace the .value()
member function with a free function
boost::constrained_value::value(), and then write something like this?

#ifndef NDEBUG
typedef bounded_int<int, 0, 100>::type my_int;
#else
typedef int my_int;
#  if IM_FINE_WITH_MACROS
#    define value(x) x
#  else
inline int value(int x) { return x; }
#  endif
#endif

Then, as long as the user always writes "value(x)", letting ADL pick
up the boost::constrained_value::value() free function, the
macro/inline function above will silently kick in instead if defined.
This gets rid of any requirements on the quality of the optimizer in
order to get performance just like an int.  Did you already try this
and find it problematic?

The note in the docs about the dangers inherent in using the library
with floating point types is well taken.  However, it would be nice if
you provided more support for such uses.  For instance, how about
providing a type that just checks for NaNs, or one that checks the
constraint every time you call .value(), so that eventually you'll
catch a range violation, even if it's masked by the underlying value
residing in a register for a while?  I have only a passing knowledge
of the issues involved, so I realize these suggestions may be naive.

However, it would be nice to have either a) something that works much
of the time, even if not perfectly, or b) a more in-depth section in
Rationale covering why, and in what cases, constrained floating point
values are doomed to fail.  B) would at least prevent every user of
the library from spinning her wheels trying to make it work for
floating point values if it never will.  I may be asking for excessive
handlholding here, I'm not sure.  It's just that "don't use built-in
floating point types with this library (until you really know what
you're doing)" is a little unsatisfying -- the nice thing about most
Boost libraries is that you don't have to have deep knowledge of the
details underlying them, because they encapsulate much of the critical
knowledge necessary for using them.

> - What is your evaluation of the implementation?

The implementation seems reasonable, but there are no tests to give me
a warm-and-fuzzy that everything works as it seems to on cursory
examination.  For instance, just glancing over the code, in
bounded.hpp, the implementations of within_bounds::is_below() and
within_bounds::is_above() appear to be wrong.  If
lower_bound_excluded() or upper_bound_excluded() is true in the
respective functions, shouldn't the result always be false?  Instead,
the *_bound_excluded() == false case in both functions has the exact
same semantics as the *_bound_excluded() == true case.  Sure enough,
when I wrote the small test app below, very similar to one of the
tutorial examples, I got an unexpected exception:

#include <iostream>
#include <boost/constrained_value.hpp>

int main()
{
    namespace cv = boost::constrained_value;

    typedef cv::bounded<
        int,
        int,
        int,
        cv::throw_exception<>,
        bool,
        bool,
        std::less<int>
    >::type b_type;
    b_type bounded(b_type::constraint_type(-5, 5, true, true));

    // prints "1 1"
    std::cout << bounded.constraint().lower_bound_excluded() << " "
              << bounded.constraint().upper_bound_excluded() << "\n"
              << std::endl;

    bounded = 0;  // ok
    bounded = -6; // throws (!)

    return 0;
}

Changing the else cases of within_bounds::is_below() and
within_bounds::is_above() to always return false fixed the problem.

I think this underscores the need for tests to be provided with
libraries submitted to Boost.  With a library as small as this one, a
complete set of tests is not even that tall an order.  Reviewers
should be able to comment on the quality of the tests, just as with
any other aspect of the implementation.

Also, in constrained.hpp, the
BOOST_DEFINE_CONSTRAINED_ASSIGNMENT_OPERATOR macro seems a little odd.
 Why are _op_ and _op_name_ passed in, when _op_ is used to create
_op_name_ via token pasting, and _op_name_ is never used?

> - What is your evaluation of the documentation?

In general, it's good.  It's clear, and covers everything well.
However, I consider the fully-generated Doxygen reference
documentation to be too detailed.  For example, as a user, why must I
know that within_bounds<LowerType, UpperType, LowerExclType,
UpperExclType, CompareType> derives from compressed_pair<LowerType,
LowerExclType>?  From my perspective, it's an implementation detail,
and therefore just noise.  I'd rather see Boostbook-integrated Doxygen
references, a la Boost.Xpressive.

Also, in "Object remembering its past extreme values" you have
bounded<> qualified by cv::, it seems without ever declaring cv.  From
your note about assuming ::boost::constrained_value everywhere, it
seems you could just leave it off.

> - What is your evaluation of the potential usefulness of the library?

It seems quite useful.  Better support for floating point values
types, along with explicit notes on when such types fall flat, would
make it even more useful.

> - Did you try to use the library?  With what compiler?  Did you have any problems?

Yes, with GCC 4.1.0.  Problem noted above.

> - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

I spent about 4 hours reading docs and implementation, and writing
small amounts of test code.

> - Are you knowledgeable about the problem domain?

Yes.

Zach Laine