<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Yeah, that AWT code is changed almost exactly the same way.
Don't know why they kept Vector, could be AWT specific reason that
doesn't apply to FX. Uncontested synchronized blocks should
however be practically free, so I don't think it's worth doing
more than copying their changes.</p>
<p>--John<br>
</p>
<div class="moz-cite-prefix">On 30/11/2024 18:51, Nir Lisker wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CA+0ynh8YPMRFQZUnjSh7=dyNsrCC9V9H3z25_0QDMGx+=X+JmQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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"
moz-do-not-send="true" class="moz-txt-link-freetext">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"
moz-do-not-send="true" class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true" class="moz-txt-link-freetext">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">
<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>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>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>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>
</blockquote>
</body>
</html>