Re: [boost] Re: Formal Review for Interval Library beginning

From: Herve Bronnimann (hbr_at_[hidden])
Date: 2002-09-03 07:50:23

Hi Gennadiy: Let me add to a few points Guillaume has already made.
I will not answer all your questions, as Guillaume has already aptly
replied.

On Mon, Sep 02, 2002 at 05:41:23AM -0400, Gennadiy Rozental wrote:
> 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.

As Guillaume mentioned, we tried to make minimum changes with Jens'
original design. Also, as I mention in the rationale:

  "These policies are packaged into a single traits class, rather than
  making interval with four template parameters. This is both for ease of
  use (the traits class can be picked by default) and for readability."

The first point (defaul classes) is equally valid with 1 combined policy
or 3 independent ones. Readability, however, will quickly suffer if you have to
put everywhere:

  template <class T, class Rounding, class Compare, class Checking>
  some_function(interval<T,Compare,Rounding,Checking> const& ....)

instead of

  template <class T, class Traits>
  some_function(interval<T,Traits> const& ....)

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

This would not be very useful, as both are used in normal code: the set
method is used internally to avoid the performance hit of checking in
internal routines where it is not warranted. The assign function,
however, is visible to the user and should perform checking.

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

In short, to avoid a copy constructor.

> 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

That would be possible, indeed it was discussed at some point. We chose
not to do that because we wanted to retain the library within a
single include statement.

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

As a matter of good style (if not anything else), all template arguments
should be accessible from a class template. This in order for library
users to access the traits. If the interval class is passed as a
template parameter, there is no other way (short of knowing the interval
declaration) of accessing the traits class. In turn, this might be
needed, eg. to access the rounding traits.

> Code:
> 
> 3. why less specialization is under if 0?

Also in the rationale, "Specialization of std::less. There is no need to
specialize std::less for intervals. Depending on the result wanted, the
comparison policy can be changed using the template constructor, which
has the effect of redefining the operator <. Hence, use the policy
compare_data to insert intervals in std::map."

> 5. Why interval implementation is located in utility.hpp header. It may be
> misleading.

The file name could be changed indeed. A relic of the past? :)

> 6. There are some commented lines. Since we are using cvs may be it worth to
> clean the code?

Maybe, but who would diff past versions to find lost comments? I find it
better to leave the comments until the corresponding issues are
resolved, IMHO.

I agree though that they should be left to a strict minimum, and
attempts made to remove them before a public release.  For this review,
we tried and removed many, but were under time constraints. The internal
code is still undergoing beautification, though, so do not lose hope! :)

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

It would be a very short page (unless you also include the definition of
the operators and functions). In that case, yes, this was considered,
and remains an option.

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

Agreed, and we share the same definitions. We kept it out of consensus.
I also favored policy name, as it indeed makes more sense. Maybe
after the review period, this will be changed. It would make the code
more readable.

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

See numbers.hpp. Although it is quite complicated to formalize the
concept and perhaps move towards concept checking. I personally don't
think it is desirable in this case. The reason are several:

As you yourself mentioned, many users will not need transcendental
functions, etc. but there are part of a requirement (namely, sin(x)
needs to work for x of type T, etc.) So with concept checking, interval
would not be usable for arithmetic computations for which the number
type neither provides nor requires sin(), etc.

Note that the current library works fine if these functions are never
called, as templates that are not instantiated are not compiled AFAIK.

It would be possible to provide a hierarchy of concepts (encompassed by
the rather vague names arithmetic, algebraic, and trigonometric number
types), but this is really the object of a separate library. BTW, I would
love to have those concepts available, as they are also required in
geometric applications, and I suspect many others.

Thanks for your comments,
-- 
Hervé