[boost] Re: Formal Review for Interval Library beginning

From: Gennadiy Rozental (gennadiy_at_[hidden])
Date: 2002-09-02 04:41:23

Ok, here first couple notes/question for the authors:

Interface:
1. What is the purpose in uniting 3 orthogonal policies parameters in one
bottle. It does not buy you anything in default case. It still would be
interval<T> whether you have 1 combined policy or 3 independent ones. While
in custom interval you require additional structure to define. It could be
even more convenient would you use named template parameters.

2. Presence both assign( l, u ) and set( l, u ) in public interface is
confusing. Maybe you could have only one method assign that in debug mode is
using checking implementation while in production non-checking. In general
you should at least try for conforming compilers to put everything after "//
the following is for internal use only" into private section of the class

3. "// The following needs to be defined in the class body for VC++.". May
be it should put it in ifdef guards?

4. Why do you need set_empty and set_whole. Why could not you use "=
interval::empty()" and "= interval whole()"

5. interval free function interface/implementation IMO should be split up in
several independent headers. For example In many cases I may not need
trigonometric, transcendental and so forth functions over intervals. It may
also allow to minimize dependency on STL headers cmath and algorithm

6. traits_type is in public interface. Should it be there?

7. All rounding staff does not defined as static. This makes you to write
something like this:
    typename Traits::rounding rnd;
     ... rnd.sub_down ...

Is there reason why are they nor static?

Code:

1. Macro BOOST_INTERVAL_DEFINE_OPERATOR_2 better be named
BOOST_INTERVAL_DECLARE_OPERATOR_2. The same for
BOOST_INTERVAL_DEFINE_COMPARISON. Also you probably what to undef them after
use.
2. Boost recommend to use T const instead of const T
3. why less specialization is under if 0?
4. using std::log and myriads of other symbols is all over rounding headers.
What about the compilers that does not put the into std namespace?
5. Why interval implementation is located in utility.hpp header. It may be
misleading.
6. There are some commented lines. Since we are using cvs may be it worth to
clean the code?

Docs:
1. Heading: May be it's worth to name the page Interval library (or
something like this) and put reference to the definition separately
somewhere.
2. On top of the page right under he header there is TOC. IMO it is aligned
a bit strangely and it would look better if you delete <center> tag
3. Introduction, statement 2: "It consists of a single header". It is not
true.
4. Introduction, statement 3: "Traits is a policy".
    After long discussions here I (and I hope others) came to very strict
definition of what is trait and what is policy and in which case which one
is used.

Trait:    type property that is inheritably belong to/defined by the type.
IOW for a given type T there only one value/definition of the
trait/property. Example numeric_traits: for given type T it uniquely defines
max(), min() and so on. Accordingly traits are never a template parameters.
Policy:  something that is orthogonal to the type definition. IOW for a
given type T you could *choose* the policy you want to use with it. Example:
allocator. Accordingly policies are always a template parameters. Since
there is no other way to let the component know about your selection.
    This long description above is presented to ground my opinion that you
template parameters should be names Policies not Traits AFAICT
5. interval is parameterized by type T. But you never rigorously specified
the requirement for this type (and it should be somewhere on top). Maybe
some kind of concept checking would also be useful.
6. More on item 5. The first statement in "Interval Arithmetic" section says
that: "An interval is a pair of numbers ".
IMO this imply that only intrinsic (plus probably something like big_int in
mind) types supposed to be used with interval? Since it is allowed to use
any totally ordered type ( or maybe even partially ordered, I did not get
why I need total order always; "due to the definition of an interval" does
not clear enough) with interval. I think that the above statement would
better be phrased something like: "An interval is a pair of ordered values".
Appropriate changes everywhere else.

Regards,

Gennadiy.