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.laur...@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 <nlis...@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.hendr...@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 >>> >>>
