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

Subject: Re: [Boost-users] [boost] [review][constrained_value] Review of Constrained Value Library begins today
From: Stjepan Rajko (stjepan.rajko_at_[hidden])
Date: 2008-12-06 12:31:21

Here is my review for the proposed library:

>
> - What is your evaluation of the design?

I think the design is simple, elegant, and allows a lot of
flexibility.  The only problem I see is that it doesn't address the
frequently requested support for floating point constraints.  And with
that, the problem is not in the implementation-related aspects of the
design, but in what the library sets out as requirements for the
behavior of the constraint.  I think permitting constraints that can
"spontaneously" switch from unsatisfied to satisfied (and documenting
what that means in terms of guarantees the library makes) would be a
good thing.

One thing I want to mention is that there is a slightly higher
abstraction that this library almost addresses, what can maybe be
called "monitored values" - values where you need to do something when
the value changes (or is first constructed).  In the case of this
library, "something" is checking a constraint and calling a policy if
the constraint is violated.  In other cases, it might be calling a
boost.signal, in another, writing to a log.

The author might consider (eventually) altering the design of the
library to offer this higher level of abstraction, as he already
solves the problem of hooking into a value change reasonably well (so,
in addition to constrained and bounded classes, offer a monitored
class using which constrained is implemented).  This would allow much
wider use of the library.  (I don't know if there are requirements for
a "MonitoredValue" library that are significantly different / in
addition to what the current libraries, that I'm not thinking of at
the moment)

> - What is your evaluation of the implementation?

I looked at the implementation of the constrained class, and found it
very satisfactory.  Well implemented, well documented code that was a
pleasure to review.

> - What is your evaluation of the documentation?

Great as a tutorial, and great as class / function reference.

The only part I found lacking is the discussion of exact requirements
on the underlying value type / constraint (my previous posts elaborate
on this).  Robert - I think the notion of "spontaneous" changes of the
constraint extends beyond the floating point case and perhaps be used
to discus other "spontaneous" changes or changes beyond the control of
whatever the library has control over (like the use_count of a
shared_ptr).

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

Very useful.  Beyond the constrained and wrapping built-in types, the
documentation suggests some very creative uses of the library, and the
flexibility of the library promises a lot more.  When it can take
advantage of declspec, the usefulness will further increase even more.
 Lambda might kick it up a notch as well (Robert, in my suggestion to
provide syntactic sugar support for the calling of a f(value_type &)
function, my unstated point was that f could be any callable -
including a lambda expression).

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

Nope.  No problems :-)

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

I read through the documentation a few times, followed the discussion
on the list, and looked at the implementation of the constrained
class.

> - Are you knowledgeable about the problem domain?
>

My experience in using functionality related to the library consists
of  inserting asserts here and there, and manually wrapping values
when they need to wrap. I recall plenty of situations where having a
library like this would have been wonderful.

> Please state in your review, whether you think the library should be
> accepted as a Boost library.

I believe the library should be accepted.  I found the library quite
feature-complete for its domain (in reading the docs, every time I
found myself thinking "this library should support x", a few
paragraphs down I would find out it does), while preserving simplicity
and elegance.

Some of the things I would definitely like to see in the final version are:

* document exact requirements on the value, constraint and policy
types, and how they relate to library's guarantees
* the library should *legally* allow for a reasonable floating point
solution (it would be great if it actually provided one, but allowing
one would be good enough for me)
* (seconding suggestions by other reviewers): provide tests (including
sizeof tests)

Nice work, Robert!

Stjepan