Re: [boost] Interval Library review

From: Sylvain Pion (pion_at_[hidden])
Date: 2002-09-08 17:57:11

Thanks for your review, Gennadiy.
I will just comment on a few points.

> 2. Open/Close intervals
> Library should explicitly support open and close intervals. For example it
> could be 2 subclasses open_interval<...> and close_interval

I still think that support for open/close intervals, as well as Multi-Interval
are things that should come independently, on top of this library.

Also, do you, or anybody, know of existing implementations of these two
extensions ?

> 4. Multi-Interval support
> Though I agree that this is complex task and need not to be in a first
> release of the library, I still believe it should be considered as essential
> part of generic interval library. In my experience static analysis task
> required namely this type of abstraction. Also, result of operation 1/x for
> x = [-1,1] should be (-inf,-1] && [1,+inf) not [-inf,+inf]. Even if it could
> be ok in numeric computation domain (Unfortunately I did not get a response
> on my request on usage examples for the library and I am not an expect in
> this domain to judge) I could envision that it is not in some others. So IMO
> library should state that this is among future directions.

Supporting "1/[-1;1] = (-inf,-1] && [1,+inf)" would be nice theoretically.
However, consider a basic real world usage : this division is probably not the
only arithmetic operation involved, and this (already catastrophic) result is
going to be fed to the following arithmetic operations.  For example, suppose
it is part of the simple function f(x) = 1/x + x.  Even with your better
multi-interval, f([-1;1]) is still going to become [-inf;inf] right after the
following addition.  So I do not find this feature important in general.
At least this example probably does not justify the complexity of dealing with
multi-intervals.

> 5. Interval lacks notion of iterator
> I believe it could be convenient in many context to consider interval as a
> container and accordingly we need the iterators.

Why not build an external iterator range from lower() and upper() as you
see fit ?  That would be much more flexible than any hard-coding that
can be done in the interval class, right ?

> 3. Why lower, upper free functions returns T instead of T const&? It could
> be expensive when you work with types like bigint

They do return const T&.  It's a bug in the doc.  Thanks for pointing it out.

> Implementation:
> 
> 2. namespace interval_lib is confusing.
>    Everything that is in this library is interval lib. Better name may be
> interval_policies.

It's not only the policies though, constants like pi are there too...

> 4. Why constant pi defined only as a double or float. What if I need pi as
> miltiprecision_double class value? Maybe it should be computed at compile
> time. You may provide precomputed specialization for intrinsic types.

It's also defined for long double (at the same IEEE precision as double for
now).  It's a job for the math constants library anyway.  Once Paul provides
nice lower/upper constants, the interval library will fetch them from there.

> 9. Function templates like: template<class T> inline T pi_lower() does not
> work at least on MSVC. Use class specializations instead.

Thomas pointed out the cleaner way I was also thinking of (I've been beaten by
this bug, and it's the kind of thing that you remember for a long time :).
Thomas' solution is better, because the return type is exactly interval then,
it's not a different class with a conversion operator.  The difference might
cause problems in some cases.
[ I'm glad to learn this easy workaround is going to work. ]

> 8. class interval implementation should be in interval.hpp or
> interval_impl.hpp or interval/impl.hpp

Definitely.

> Compilation
> Its a pity that library does not compile with MSVC 6.5. It does not seem to
> use any "advanced" features so should be pretty portable.

None of us three had the possibility to test under VC++.
Guillaume started to test Borland's compiler IIRC.
I did the port to SunPRO.
Adding support for VC++ blindly (without direct access) is probably not very
productive (that's my experience with porting CGAL in general).  Better get
access to one, in order to be sure to fix all problems correctly.

-- 
Sylvain