Re: [boost] [Polygon] review - interval

Subject: Re: [boost] [Polygon] review - interval
From: Simonson, Lucanus J (lucanus.j.simonson_at_[hidden])
Date: 2009-08-30 00:33:30

Steven Watanabe wrote:
> interval_concept.hpp:75
> It would make more sense to me to make it a precondition
> of construct that low_value <= high_value.

Enforced by excpetion or unchecked?  Maintaining that invariant is the whole purpose for the interval, so I'm not sure putting the responsibility onto the caller is the right choice.

> I would like to see a specialization of interval_traits
> for boost::interval.

I could add an example.

> interval_concept.hpp:190-191
> I don't think this is correct. Consider calling
> flip with an interval [1,2] and an axis 1. I would
> expect the result to be [0,1]. flip as written will
> give [1-2,1-1]=[-1,0]. The correct formula is
> newLow = 2*axis - high(interval);
> newHigh = 2*axis - low(interval);

Yikes, good catch.  You're right.
 
> interval_concept.hpp:205-206,219-220,231-232,246-247, maybe more
> Please use static_cast, instead of a C-style cast.

Many more.  I use c-style casts for coordinate type conversion pretty much uniformly.  How big of an issue is this?  It would take a while to hunt them all down.

> interval_concept.hpp:247
> Is it necessary to set high in terms of low instead of
> adding displacement to high? What's the difference
> between convolve and move?

Changing low in the previous line may change high because it maintains the invariant that low <= high.  If low were set to something larger than high the high is changed to be equal to low.  There is no difference between convolve against a coordinate and move by a displacement.  The convolution system of functions is not very intuitive to use, however, so I provide move for usibility.  I guess technically there is a difference in that move uses displacement_type where as convolve uses coordinate_type for the argument.

> interval_concept.hpp:366
> I'm inclined to doubt whether this is any faster than
> if(position < low(interval)) return low(interval) - position;
> else if(position <= high(interval)) return 0;
> else return(position - high(interval));
> which is easier to understand.

I like writing code without branch instructions, I get a kick out of it.  Whether it is faster depends on a lot of factors.  It can be faster.

> interval_concept.hpp:396
> is the use of the bitwise & operator instead of
> the logical operator intentional?

Yes.  If the inlining happens correctly it is more expensive to have a branch than to execute both compares between integers.  This is true of all pipelined architectures.

> I didn't see get_half and join_with in the documentation.

They are something of implementation details.  I chose not to document them.

Thanks,
Luke