Hmmm...
In general, if you can, you should write a short regression test and
push it with your checkin. I probably should have mentioned that before
you pushed since these bugs sound like they are conducive to a tiny
automatic regression test.
At this point we'd need a new bugid to push any regression tests. Is
there another bugid associated with this general set of problems that we
could use to generate them and push them?
...jim
On 1/18/11 11:02 AM, Denis Lila wrote:
Hi Jim.
Actually I was thinking it would be OK to leave the endpoints in local
variables, but it's not worth twiddling with at this point. The
changes
look great and good to go.
Nice! I will push later today.
Were these intended to be separate pushes? Are the independent bugs?
Do you need me to massage any bugids for you?
They are 2 separate, independent bugs, so I think it would be best
if they were separate pushes. I think the bugids I already have are
ok.
Thanks for reviewing,
Denis.
----- Original Message -----
Hi Denis,
...jim
On 1/13/11 9:50 AM, Denis Lila wrote:
Hi Jim.
One general comment about the new helper method. I probably
wouldn't
bother loading the control points into local variables since you
only
use them once in the function. It might be wasted effort if the
cubic
function isn't called, and meanwhile you are forcing the compiler
to
find some local storage to stuff them into for no good reason (the
compiler can't optimize those fetches out or around since it has no
concept of the potential side effects, or lack thereof, of calling
the
abstract getters)...
Also, on the testing of the return value, I wouldn't bother with
testing
"% 2". If you look at Path2D it just assumes that it is an even
number
(or the INTERSECT constant) and does the test based on whether it
is
INTERSECT or non-zero (for WIND_NON_ZERO which is compatible with
CubicCurve and QuadCurve - I don't think there can be interior
holes
in
a either single curve's outline)...
I did both. I updated the webrevs:
http://icedtea.classpath.org/~dlila/webrevs/containsFix/webrev/
http://icedtea.classpath.org/~dlila/webrevs/intersectsFix/webrev/
Thank you,
Denis.
