[OpenJDK 2D-Dev] CubicCurve2D.solveCubic and containment/intersection bugs.

[Jim Graham]

Hi Denis,

You make a lot of local variables final - is that for performance?  Does 
it actually have any impact (I've only ever used final on a local 
variable when it was required for an anonymous inner class to work).

Line 1104, I though we were going to add a comment that clarified that 
the values we computed were actually p/3 and q/2.

Line 1123 - it has no bearing on the correctness, but logically I think 
this would read better as "eqn = Arrays.copyOf(eqn, 4)" since it is 
"saving eqn", but the way you wrote it looks like it is "saving res" 
(which happens to == eqn).

Line 1131 - {}

Line 1142 - that's a huge error allowance.  Do we really need that much?

Lines 1178 - 1180 - only used inside "num == 3" case?
Lines 1182,1183 - only used in "num == 3, critcount == 1" case?

Line 1182 - I don't understand why this is -1 or 1, why not solveEqn? 
Or is it because you only care about the sign of the function there?  Is 
it really that predictable?  Also, what about just using "-eqn[3]" as 
the "sign approximator"?  (Not helpful if eqn[3] == 0, but we wouldn't 
be here if that was the case.)

Line 1192 - this case ends up returning "1", but can't we enter here 
with 2 roots?  "critCount==1" can happen if there are 2 roots, no?

Line 1179 - isn't that already done in getRUB()?  (And see my comment 
about eliminating the "max()")

Line 1206 - I don't understand what this case is trying to accomplish?

Line 1219 - We can get here if "num == 2, critCount == 1" which means we 
may actually have 2 valid roots, no?

Line 1230 - could be declared on line 1233?

Line 1283 - move this just inside the while?  (And eliminate 1295?)

Line 1340 - M += 1 + ulp(M) would save a max operation and work as well, 
wouldn't it?

Line 1386 - It looks like you moved this function from below which just 
generates some unnecessary diffs - is there a reason for that?

			...jim

On 1/25/2011 3:45 PM, Denis Lila wrote:
>>  From that email on we started diving into the containment methods
>> instead of solveCubic and the email you refer to doesn't have a webrev
>> link to the code with your changes. Is there a final webrev for the
>> solveCubic changes?
>
> I made a webrev, along with 3 regression tests (2 for my previous 2 pushes
> and one for solveCubic):
> http://icedtea.classpath.org/~dlila/webrevs/cc2d/webrev/
>
> The regression test for solveCubic just tests the equation {0, 0, 1, 1}
> for which we used to find only 1 root. I thought about including a
> randomized stress test based your trySolve method that you sent a while
> ago, but I'm not a big fan of regression tests that have a small chance of
> failing even when nothing is wrong. What do you think about this?
>
> Regards,
> Denis.