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

From: Guillaume Melquiond (gmelquio_at_[hidden])
Date: 2002-09-02 07:10:42

On Mon, 2 Sep 2002, Gennadiy Rozental wrote:

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

The purpose was to define an interval by two parameters. The first one is
the base number type. The second one is all the policies.

Since the three policies are orthogonal, it was possible to define the
interval class as :

  template<class T, class Compare, class Rounding, class Checking>
  class interval;

instead of :

  template<class T, class Policies> class interval;

However, an interval class with four parameters instead of two parameters
is a bit less easy to manipulate. Mainly because the policies are not set
at the beginning of a program but are free to evolve depending of the
situation.

I agree with you concerning the last point; named template parameters can
be interesting. But I'm not sure it's what the users will principally
used. I think they will use things like:

  typedef change_compare_policy
            <my_old_interval_type, my_new_compare_policy>
          my_new_interval_type;

in order to change a policy on a one-of-a-kind basis.

To avoid interface bloat, templates like change_compare_policy are not in
the current library. But if enough people are interested, they could be
added.

> 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

Please note that set(l,u) is not in the published interface of the
library. The only documented function is assign. set(l,u) should not
be used outside the library. It is only intended for internal use and
should be in a private section. The reason it is in the public section is
only an artefact of the way the library interface is designed and the
compilers are conforming.

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

So you propose to have two times the same function, one time inside the
class body for VC++ and the other time outside for the others compilers?
In my opinion, it is a good example of code duplication and I would like
to avoid it whenever possible.

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

As for set before, if you refer to the documentation, you will see that
set_empty and set_whole are not part of the published interface of the
class.

> 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

Yes, it is a possibility.

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

Why not? Is it really a problem to let the user access something he/she
has him/herself defined?

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

Yes, there is.

First of all, with a protected rounding mode, no function should ever
globally change the rounding mode. So it must be saved at the beginning of
a function and restored at the end of it. Is there a better place than the
constructor and destructor of Traits::rounding to do such an operation?

Moreover, since the rounding is locally set, a function like sub_down may
need to access this local information. If this function is required to be
static, it's no more possible.

Let's see an example. Let's suppose, you are writing a version which tries
to cache rounding mode change. It will look like:

  struct my_rounding {
    my_rounding()  { save_rounding(); current_rnd = RM_UNK; }
    ~my_rounding() { restore_rounding(); }
    sub_down(T x, T y) {
      if (current_rnd != RM_DN) set_rounding(RM_DN);
      return x - y;
    }
   private:
    typedef enum { RM_UNK, RM_UP, RM_DN } rounding_mode;
    rounding_mode current_rnd;
  };

I don't think it is possible to write the same class if the methods are
static. There is a lot of situation where it is necessary to access local
data.

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

Yes, the name could be changed.

Please note that they already are undefined after use (interval.hpp:142
and interval.hpp:181).

> 2. Boost recommend to use T const instead of const T

Ok. (if you remember where it is written, could you give me a
pointer? thank you)

> 3. why less specialization is under if 0?

Because it is not used. However, if somebody has a special case that need
it, we can remove the if 0. If nobody is interested, it will be deleted at
the end of the review.

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

It's a big problem. It's the reason why interval/detail/bug.hpp tests if
BOOST_NO_STDC_NAMESPACE is defined.

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

If it is really disturbing, it can easily be moved elsewhere.

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

Yes it is worth to clean the code of the few remaining commented
lines. But what do you mean by "Since we are using cvs" ?

> 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

Nothing much to say.

> 3. Introduction, statement 2: "It consists of a single header". It is not
> true.

Yes. The reason of this error is that only one header is needed to be
included in order to access the major part of the library.

> 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

Yes, you are right. And it's reason why we speak of "policies" and not of
"traits" in the documentation. However, as you may know, this library was
originally the work of Jens Maurer, and by respect to that work, we tried
to limit the number of naming changes to a minimum.

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

The requirements for the type are described in numbers.htm, accessible by
the topic "Base number type requirements" in the TOC. I don't know if you
are speaking about this page.

Some concept checking would be nice. But I'm not sure it would be that
useful (am I underestimating the strength of concept checking in the case
of interval arithmetic?)

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

No, it's really not enough if it's only a partial order. I may be wrong
but if only the bounds are ordered, you will have a problem. Indeed, you
need to know if another number is inside or outside the interval, since an
interval represents all the numbers between the two bounds. The interval
arithmetic not only manipulate the bounds, but it is also meant to
manipulate all the numbers between the bounds.

In fact, you are right, some partial orders could be used to do interval
arithmetic. However, not all partial order are usable. For example, what
will you do with complex numbers and the natural order on the real numbers
(which is a partial order on the complex numbers)? The interval arithmetic
will only defined when the bounds are ordered; so it will only be
defined on the real numbers (where the order is total).

> Regards,
> 
> Gennadiy.

Thank you for your comments,

Guillaume