Possible mistakes in com.sun.javafx.geom.AreaOp

[Nir Lisker]

You're right, these geometry classes were copied from AWT. Here is the
offending AreOp:
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/geom/AreaOp.java
.

They generified their code at some point like John did. They also just
create a new vector in the 'numedges' check. I wonder why they kept Vector
though.

On Sat, Nov 30, 2024 at 7:43 PM Laurent Bourgès <bourges.laurent at gmail.com>
wrote:

> Probably derived from java.awt.Area:
>
> https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/geom/Area.java
>
> Le sam. 30 nov. 2024, 17:59, Nir Lisker <nlisker at gmail.com> a écrit :
>
>> I agree with your type analysis. However, I'm hesitant to change the
>> logic in regarding 'numedges' without someone who is familiar with the
>> domain taking a look; these sorts of computations often have non-obvious
>> sides to them.
>>
>> I also think that the class was copied. The raw types usage suggests a
>> 1.4 era. Vector itself suggests even earlier times, perhaps AWT. If the
>> code doesn't require thread synchronization, and I don't think it does,
>> using List could even speed it up a bit.
>>
>> Do you have an idea about the needless 'toArray' calls? The return is not
>> captured and it doesn't have side effects. Maybe to check the types?
>>
>> On Sat, Nov 30, 2024 at 10:25 AM John Hendrikx <john.hendrikx at gmail.com>
>> wrote:
>>
>>> Hi Nir,
>>>
>>> I encountered that class before while doing raw warning clean-ups in
>>> graphics (which were never integrated).
>>>
>>> The problem IMHO is in the assignment in `calculate`:
>>>
>>> edges = pruneEdges(edges);
>>>
>>> This assignment is both confusing and unnecessary, and violates the
>>> principle of re-using variables for different purposes.  Method pruneEdges
>>> must return CURVES (the name even implies it -- remove edges), and the
>>> debug code in this block even assumes it does (it assigned `numcurves` to
>>> `edges.size()` then assumes that it contains curves when converting to the
>>> array format):
>>>
>>> Rewrite the code as follows and it will be much clearer:
>>>
>>> public Vector<Curve> calculate(Vector<Curve> left, Vector<Curve> right)
>>> {
>>>
>>> Vector<Edge> edges = new Vector<>();
>>>
>>> addEdges(edges, left, AreaOp.CTAG_LEFT);
>>>
>>> addEdges(edges, right, AreaOp.CTAG_RIGHT);
>>>
>>> Vector<Curve> curves = pruneEdges(edges);
>>>
>>> if (false) {
>>>
>>> System.out.println("result: ");
>>>
>>> int numcurves = curves.size();
>>>
>>> Curve[] curvelist = (Curve[]) edges.toArray(new Curve[numcurves]);
>>>
>>> for (int i = 0; i < numcurves; i++) {
>>>
>>> System.out.println("curvelist["+i+"] = "+curvelist[i]);
>>>
>>> }
>>>
>>> }
>>>
>>> return curves;
>>>
>>> }
>>>
>>> Then the initial code in `pruneEdges` is just plain wrong, and seems to
>>> be an attempt at optimization done incorrectly by returning an existing
>>> vector to save having to allocate a new one:
>>>
>>> private Vector<Curve> pruneEdges(Vector<Edge> edges) {
>>>
>>> int numedges = edges.size();
>>>
>>> if (numedges < 2) {
>>>
>>> return edges;
>>>
>>> }
>>>
>>> Method pruneEdges however is supposed to return a minimum set of curves
>>> that enclose an area.  A single edge can't contribute any area. I'm pretty
>>> sure that if you change the check to `numedges < 1` that if the remaining
>>> code runs normally it would also come to that conclusion (it would return
>>> an empty vector of curves).  So this "optimization" here is doing the wrong
>>> thing by potentially returning a single edge instead of always returning an
>>> empty curve vector.
>>>
>>> Now, I'm pretty sure we'll never see this case in practice, due to other
>>> checks being done (specifically the `getOrder() > 0` check when adding
>>> edges) and the likely fact that there is always going to be a minimum of 2
>>> curves being passed to `calculate.  Adding an assert or just modifying the
>>> code, and then running all tests may help verify this.  I believe however
>>> that the code needs to be this:
>>>
>>> private Vector<Curve> pruneEdges(Vector<Edge> edges) {
>>>
>>> int numedges = edges.size();
>>>
>>> if (numedges < 2) {
>>>
>>> return numedges == 0 ? (Vector<Curve>)(Vector<?>)edges : new Vector();
>>> // as a single edge can't encompass any area, a single edge also results in
>>> no curves being returned
>>>
>>> }
>>>
>>> The reason why we do need to return a mutable Vector here is because the
>>> classes using the return value of calculate assume that it is writable and
>>> doesn't need copying.  The ugly double cast of edges is relatively
>>> contained -- it is only casted when empty (to avoid creating another vector
>>> instance), and it was created by this class so its under control.  Always
>>> returning `new Vector()` would also work, but it may perform a tiny bit
>>> slower in some cases (probably not measurable IMHO).
>>>
>>> I suspect the entire class is copied from somewhere, as at the time
>>> JavaFX was written using Vector while at the same time trying to do these
>>> kinds of optimizations is odd to say the least.
>>>
>>> --John
>>>
>>>
>>> On 30/11/2024 00:12, Nir Lisker wrote:
>>>
>>> I came across a potential mistake in the
>>> class com.sun.javafx.geom.AreaOp. It uses raw Vector types and while trying
>>> to add generic parameters there for type safety, I got some conflicts.
>>>
>>> In the method AreaOp::calculate, the arguments should be Vector<Curve>
>>> and the return type should also be Vector<Curve>, but it returns a Vector
>>> called 'edges'. 'edges' is passed to the 'addEdges' method that should
>>> take Vector<Edge> and Vector<Curve>. This means that 'edges' is a
>>> Vector<Edge> and not Vector<Curve>. Already a conflict.
>>> Then it is passed to 'pruneEdges', which, if it takes and returns
>>> Vector<Edge>, runs into a conflict with the return type: 'Vector ret' has
>>> Curve added to it. If I try to return Vector<Curve> instead, then in the
>>> 'numedges < 2' check the input Vector can't be returned unless it's also
>>> Vector<Curve>, which again causes a conflict in 'calculate'.
>>>
>>> There are also 'toArray' calls that seem to not do anything, like in
>>> 'resolveLinks' (line 454) and in 'finalizeSubCurves' (429).
>>>
>>> Can anyone who knows this part of the code take a look?
>>>
>>> - Nir
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241130/380085e5/attachment-0001.htm>