<div dir="ltr">You're right, these geometry classes were copied from AWT. Here is the offending AreOp: <a href="https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/geom/AreaOp.java">https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/geom/AreaOp.java</a>.<div><br></div><div>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.</div></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Sat, Nov 30, 2024 at 7:43 PM Laurent Bourgès <<a href="mailto:bourges.laurent@gmail.com">bourges.laurent@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><p dir="ltr">Probably derived from java.awt.Area:<br>
<a href="https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/geom/Area.java" target="_blank">https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/geom/Area.java</a></p>
<br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le sam. 30 nov. 2024, 17:59, Nir Lisker <<a href="mailto:nlisker@gmail.com" target="_blank">nlisker@gmail.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.<div><br></div><div>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.</div><div><br></div><div>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?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Nov 30, 2024 at 10:25 AM John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" rel="noreferrer" target="_blank">john.hendrikx@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div>
<p>Hi Nir,<br>
</p>
<p>I encountered that class before while doing raw warning clean-ups
in graphics (which were never integrated).</p>
<p>The problem IMHO is in the assignment in `calculate`:<br>
</p>
<p><span style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px"><span style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><span style="color:rgb(0,0,0)"> edges = </span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">pruneEdges</span><span style="color:rgb(0,0,0)">(edges);</span></span></span><br>
</p>
<p>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):<br>
</p>
<p>Rewrite the code as follows and it will be much clearer:</p>
<div style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px">
<div style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">public</span><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">Vector<Curve></span><span style="color:rgb(0,0,0)"> calculate(Vector<Curve> left, Vector<Curve> right) {</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> Vector<Edge> edges = </span><span style="color:rgb(0,0,160);font-weight:bold">new</span><span style="color:rgb(0,0,0)"> Vector<>();</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,0);font-style:italic">addEdges</span><span style="color:rgb(0,0,0)">(edges, left, AreaOp.</span><span style="color:rgb(0,0,192)">CTAG_LEFT</span><span style="color:rgb(0,0,0)">);</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,0);font-style:italic">addEdges</span><span style="color:rgb(0,0,0)">(edges, right, AreaOp.</span><span style="color:rgb(0,0,192)">CTAG_RIGHT</span><span style="color:rgb(0,0,0)">);</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> Vector<Curve> curves = </span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">pruneEdges(edges)</span><span style="color:rgb(0,0,0)">;</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">if</span><span style="color:rgb(0,0,0)"> (</span><span style="color:rgb(0,0,160);font-weight:bold">false</span><span style="color:rgb(0,0,0)">) </span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">{</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> System.</span><span style="color:rgb(0,0,192);text-decoration:underline wavy rgb(244,200,45)">out</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">.println(</span><span style="color:rgb(42,0,255);text-decoration:underline wavy rgb(244,200,45)">"result: "</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">);</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> </span><span style="color:rgb(0,0,160);font-weight:bold;text-decoration:underline wavy rgb(244,200,45)">int</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> numcurves = curves.size();</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> Curve[] curvelist = (Curve[]) edges.toArray(</span><span style="color:rgb(0,0,160);font-weight:bold;text-decoration:underline wavy rgb(244,200,45)">new</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> Curve[numcurves]);</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> </span><span style="color:rgb(0,0,160);font-weight:bold;text-decoration:underline wavy rgb(244,200,45)">for</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> (</span><span style="color:rgb(0,0,160);font-weight:bold;text-decoration:underline wavy rgb(244,200,45)">int</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> i = 0; i < numcurves; i++) {</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> System.</span><span style="color:rgb(0,0,192);text-decoration:underline wavy rgb(244,200,45)">out</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">.println(</span><span style="color:rgb(42,0,255);text-decoration:underline wavy rgb(244,200,45)">"curvelist["</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">+i+</span><span style="color:rgb(42,0,255);text-decoration:underline wavy rgb(244,200,45)">"] = "</span><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">+curvelist[i]);</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> }</span></p><p style="margin:0px"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)"> }</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(127,0,85);background-color:rgb(212,212,212);font-weight:bold">return</span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)"> curves;</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> }</span></p></div>
</div>
<p></p>
<p>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:</p>
<div style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px">
<div style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">private</span><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">Vector<Curve></span><span style="color:rgb(0,0,0)"> pruneEdges(Vector<Edge> edges) {</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">int</span><span style="color:rgb(0,0,0)"> numedges = edges.size();</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">if</span><span style="color:rgb(0,0,0)"> (numedges < 2) {</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(127,0,85);background-color:rgb(212,212,212);font-weight:bold">return</span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)"> </span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212);text-decoration:underline wavy rgb(255,0,128)">edges</span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">;</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> }</span></p></div>
</div>
<p></p>
<p>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.</p>
<p>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:</p>
<div style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px">
<div style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">private</span><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">Vector<Curve></span><span style="color:rgb(0,0,0)"> pruneEdges(Vector<Edge> edges) {</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">int</span><span style="color:rgb(0,0,0)"> numedges = edges.size();</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">if</span><span style="color:rgb(0,0,0)"> (numedges < 2) {</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(127,0,85);background-color:rgb(212,212,212);font-weight:bold">return</span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)"> numedges == 0 ? (Vector<Curve>)</span><span style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px"><span style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><span style="color:rgb(0,0,0);text-decoration:underline wavy rgb(244,200,45)">(Vector<?>)</span></span></span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">edges : new Vector()</span><span style="color:rgb(0,0,0);background-color:rgb(212,212,212)">; // as a single edge can't encompass any area, a single edge also results in no curves being returned</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> }</span></p></div>
</div>
<p></p>
<p>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).<br>
</p>
<p>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.<br>
</p>
<p>--John</p>
<p><br>
</p>
<div>On 30/11/2024 00:12, Nir Lisker wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div>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'.</div>
<div><br>
</div>
<div>There are also 'toArray' calls that seem to not do
anything, like in 'resolveLinks' (line 454) and in
'finalizeSubCurves' (429).</div>
<div><br>
</div>
<div>Can anyone who knows this part of the code take a look?</div>
<div><br>
</div>
<div>- Nir</div>
</div>
</blockquote>
</div>
</blockquote></div>
</blockquote></div>
</blockquote></div>