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

[Nir Lisker]

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/f89d960a/attachment-0001.htm>