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

Subject: Re: [boost] [review][constrained_value] Review of Constrained ValueLibrary begins today
From: Robert Kawulak (robert.kawulak_at_[hidden])
Date: 2008-12-01 18:30:25

Hi Zach,

> From: Zach Laine

> 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.

Yes, the assumption is that bounded objects have two bounds, not only one.
However, you may easily achieve what you ask for:

typedef bounded_int<int, 0, boost::integer_traits<int>::const_max>::type
non_negative_int;

> Why not replace the .value()
> member function with a free function
> boost::constrained_value::value(), and then write something like this?

I think Sebastian has already explained the problem. However, if you really want
to avoid using unconstrained, you may use explicit casts instead of value()
member function:

#ifndef NDEBUG
typedef bounded_int<int, 0, 100>::type my_int;
#else
typedef int my_int;
#endif

// works with x as either bounded_int or int
// without the need to call value()
my_int y = std::max(static_cast<int>(x), 2);

Yes, I know it's not perfect. I'd rather use unconstrained too. :P

> 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?

There's already an assert every time a constrained object is modified -- is it
something that you mean here?

>  I have only a passing knowledge
> of the issues involved, so I realize these suggestions may be naive.
[snip more on the topic of FP]

I'm afraid my knowlegde in this area is also not sufficient, so I don't even
want to open this can of worms. I think that even a partial support for FP would
encourage people to use it without full understanding, and sooner or later this
may put them into trouble. I'd rather leave the warning shouting "don't use
built-in floating point types with this library (until you really know what
you're doing)" to encourage people to look for other solutions than FP.

Anyway, if there are some FP arithmetic experts out there -- you are welcome to
express your opinion. ;-)

> 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.

As I mentioned before, detailed regression tests will be added after the review.
Currently the examples file (libs\constrained_value\doc\src\examples.cpp) can be
used instead -- it uses most of the functionality of the library and should
compile and run returning 0.

> 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?

No, it shouldn't. Why do you think so?

> Instead,
> the *_bound_excluded() == false case in both functions has the exact
> same semantics as the *_bound_excluded() == true case.

It seems you've misunderstood the point. If a bound is excluded then it means
that the range is open. For example, if the upper bound is excluded, the range
is [lower, upper). If none of the bounds are excluded, the range is [lower,
upper]. Excluding a bound doesn't mean the bound does not exist, it means its
value does not belong to the range. Does this clarify your doubts?

> Sure enough,
> when I wrote the small test app below, very similar to one of the
> tutorial examples, I got an unexpected exception:
[snip code]
>     b_type bounded(b_type::constraint_type(-5, 5, true, true));
[snip more code]
>     bounded = -6; // throws (!)

I wouldn't say the exception is unexpected, your allowed range is (-5, 5) and
you assign -6. The library does exactly what it is supposed to.

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

Sure it "fixes" the problem, because then any value is correct (is NOT below the
lower bound and is NOT above the upper bound => is within the allowed range).
;-)

> 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?

_op_name_ is used in the documentation comment and is needed so Doxygen
generates proper output. Otherwise the %= operator would have invalid
documentation, since % is Doxygen's special character and must be escaped.

> 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 agree with you. If you know which Doxygen option turns showing private base
classes off, let me know. ;-)

> I'd rather see Boostbook-integrated Doxygen
> references, a la Boost.Xpressive.

Using Doxygen final output instead of the Boostbook one was a conscious
decision. The reasons are:
- Boostbook does not support some of Doxygen tags used in the code (e.g.,
@param) and leaves those sections of text out without even a warning, similarily
it leaves out brief descriptions (at least in some cases);
- personally, I find it way easier to navigate through the documentation created
by Doxygen than Boostbook, and it is also much more readable to me due to its
layout and formatting.

> 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.

Thanks for spotting this, to be fixed.

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

Thank you for your time and your vote. ;-)

Best regards,
Robert