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

[Jim Graham]

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.