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:
publicVector<Curve>calculate(Vector<Curve> left, Vector<Curve> right) {
Vector<Edge> edges = newVector<>();
addEdges(edges, left, AreaOp.CTAG_LEFT);
addEdges(edges, right, AreaOp.CTAG_RIGHT);
Vector<Curve> curves = pruneEdges(edges);
if(false) {
System.out.println("result: ");
intnumcurves = curves.size();
Curve[] curvelist = (Curve[]) edges.toArray(newCurve[numcurves]);
for(inti = 0; i < numcurves; i++) {
System.out.println("curvelist["+i+"] = "+curvelist[i]);
}
}
returncurves;
}
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:
privateVector<Curve>pruneEdges(Vector<Edge> edges) {
intnumedges = edges.size();
if(numedges < 2) {
returnedges;
}
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:
privateVector<Curve>pruneEdges(Vector<Edge> edges) {
intnumedges = edges.size();
if(numedges < 2) {
returnnumedges == 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
