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.
