- 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