Kernel: Collinear_3: Static Analysis #5242
Conversation
| @@ -77,32 +77,18 @@ class Collinear_3 | |||
| upper_bound_1 = max1; | |||
|
|
|||
| int int_tmp_result; | |||
| if (lower_bound_1 < 5.00368081960964635413e-147) | |||
|
|
|||
| if ( (lower_bound_1 < 5.00368081960964635413e-147) || (upper_bound_1 > 1.67597599124282407923e+153) ) | |||
| return Base::operator()(p, q, r); | |||
There was a problem hiding this comment.
IIUC, if all the points have the same x coordinate, we end up here, it seems a bit too early to go to the exact computation, we could check the other projections. Maybe that falls under "further suggested improvements".
There was a problem hiding this comment.
I agree with Marc: the static filter should only end up on the exact computation if all the possible filtering have failed. The raison d'être of those static filters is to quickly find a reason to tell that the points are not collinear.
Points p, q and r and collinear if and only if the two vector pq and pr are collinear, that is if and only if their cross-product is null. The cross-product has three coordinates that are each 2×2 determinants. That gives exactly three opportunities for the static filters to prove that the three points are not collinear. The return Base::operator()(p, q, r); should be only at the end, when all three opportunities have failed.
There was a problem hiding this comment.
Maybe that falls under "further suggested improvements".
I meant what you suggest for collinearC3(). Also I am wondering if we could not profit from a function is_determinant_zero() which would save us a subtraction.
There was a problem hiding this comment.
Also I am wondering if we could not profit from a function
is_determinant_zero()which would save us a subtraction.
We have sign_of_determinant, although compilers may not be perfect at simplifying sign(double)==0.
There was a problem hiding this comment.
In fact I was wondering about that for rational types.
There was a problem hiding this comment.
Indeed, and for rationals, cmp can be quite a bit more expensive than ==.
| else | ||
| return Base::operator()(p, q, r); | ||
| } | ||
| if ((double_tmp_result > eps) || (double_tmp_result < -eps)) |
There was a problem hiding this comment.
you changed the next one to use abs()>eps but not this one? (not CGAL::abs so it would use sse2fabs on windows?)
we could move the computation of double_tmp_result just before this line, though it probably doesn't change anything, so not necessary.
lrineau
added
the
rm only: ready for master
lrineau
removed
Ready to be tested
Under Testing
rm only: ready for master
Summary of Changes
Make the changes as suggested by @mglisse to fix the issue. I didn't address the further suggested improvements.
Release Management