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

[Laurent Bourgès]

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/1b7ca002/attachment.htm>