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