[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] JDK9: RFR: 8039342: Fix raw and unchecked warnings in sun.awt.*
From:       Henry Jen <henry.jen () oracle ! com>
Date:       2014-04-25 18:12:19
Message-ID: 535AA583.9050002 () oracle ! com
[Download RAW message or body]

Thanks for the reviw.

On 04/23/2014 02:37 PM, Jim Graham wrote:
> Shouldn't Order2, lines 50,77 be "<Curve>"?
> Ditto for Order3, lines 56,108?
>
> I don't think we ever use these methods with any other type of Vector,
> but I guess I can see that the choice you made might be a more general
> choice.
>

Right, I took a more general approach.

> This concludes my review of the geom files per Phil's request.  They
> look fine, other than my suggestions for returning an empty curve list
> from pruneEdges and the above comments...
>

I changed to return empty curve list, but not static one as there is no 
immutable Vector I can use without implementing one.

While I see the result is private member curves in java.awt.geom.Area, I 
am not 100% certain that this cannot be leaked. I am afraid it get 
tainted somehow thus I make a new instance just to be cautious. However, 
if you feel strongly it's fine to return a static instance, we can do that.

Following is the diff to address your comments, let me know if other 
changes are needed.

Cheers,
Henry

> diff --git a/src/share/classes/sun/awt/geom/AreaOp.java b/src/share/classes/sun/awt/geom/AreaOp.java
> --- a/src/share/classes/sun/awt/geom/AreaOp.java
> +++ b/src/share/classes/sun/awt/geom/AreaOp.java
> @@ -198,11 +198,8 @@
>      private Vector<Curve> pruneEdges(Vector<Edge> edges) {
>          int numedges = edges.size();
>          if (numedges < 2) {
> -            Vector<Curve> rt = new Vector<>();
> -            for (Edge edge: edges) {
> -                rt.add(edge.getCurve());
> -            }
> -            return rt;
> +            // empty vector is expected with less than 2 edges
> +            return new Vector<>();
>          }
>          Edge[] edgelist = edges.toArray(new Edge[numedges]);
>          Arrays.sort(edgelist, YXTopComparator);
> diff --git a/src/share/classes/sun/awt/geom/Order2.java b/src/share/classes/sun/awt/geom/Order2.java
> --- a/src/share/classes/sun/awt/geom/Order2.java
> +++ b/src/share/classes/sun/awt/geom/Order2.java
> @@ -47,7 +47,7 @@
>      private double ycoeff1;
>      private double ycoeff2;
>
> -    public static void insert(Vector<? super Order2> curves, double tmp[],
> +    public static void insert(Vector<Curve> curves, double tmp[],
>                                double x0, double y0,
>                                double cx0, double cy0,
>                                double x1, double y1,
> @@ -74,7 +74,7 @@
>                      tmp[i1 + 4], tmp[i1 + 5], direction);
>      }
>
> -    public static void addInstance(Vector<? super Order2> curves,
> +    public static void addInstance(Vector<Curve> curves,
>                                     double x0, double y0,
>                                     double cx0, double cy0,
>                                     double x1, double y1,
> diff --git a/src/share/classes/sun/awt/geom/Order3.java b/src/share/classes/sun/awt/geom/Order3.java
> --- a/src/share/classes/sun/awt/geom/Order3.java
> +++ b/src/share/classes/sun/awt/geom/Order3.java
> @@ -53,7 +53,7 @@
>      private double ycoeff2;
>      private double ycoeff3;
>
> -    public static void insert(Vector<? super Order3> curves, double tmp[],
> +    public static void insert(Vector<Curve> curves, double tmp[],
>                                double x0, double y0,
>                                double cx0, double cy0,
>                                double cx1, double cy1,
> @@ -105,7 +105,7 @@
>          }
>      }
>
> -    public static void addInstance(Vector<? super Order3> curves,
> +    public static void addInstance(Vector<Curve> curves,
>                                     double x0, double y0,
>                                     double cx0, double cy0,
>                                     double cx1, double cy1,

Cheers,
Henry


>              ...jim
>
> On 4/7/14 1:46 PM, Henry Jen wrote:
>> Hi,
>>
>> Please review the webrev cleans up raw and unchecked warnings in sun.awt,
>>
>> http://cr.openjdk.java.net/~henryjen/jdk9/8039342/0/webrev/
>>
>> The following changes in AreaOp::pruneEdges() is particular worth
>> attention, when numedges < 2, two different type are mixed up in the
>> past with use of rawtypes; However, I think it could only work if the
>> Vector is empty?
>>
>> Cheers,
>> Henry
>>
>>
>>> @@ -193,16 +193,20 @@
>>>              }
>>>              return 1;
>>>          }
>>>      };
>>>
>>> -    private Vector pruneEdges(Vector edges) {
>>> +    private Vector<Curve> pruneEdges(Vector<Edge> edges) {
>>>          int numedges = edges.size();
>>>          if (numedges < 2) {
>>> -            return edges;
>>> +            Vector<Curve> rt = new Vector<>();
>>> +            for (Edge edge: edges) {
>>> +                rt.add(edge.getCurve());
>>>          }
>>> -        Edge[] edgelist = (Edge[]) edges.toArray(new Edge[numedges]);
>>> +            return rt;
>>> +        }
>>> +        Edge[] edgelist = edges.toArray(new Edge[numedges]);
>>>          Arrays.sort(edgelist, YXTopComparator);
>>>          if (false) {
>>>              System.out.println("pruning: ");
>>>              for (int i = 0; i < numedges; i++) {
>>>                  System.out.println("edgelist["+i+"] = "+edgelist[i]);
>>
[prev in list] [next in list] [prev in thread] [next in thread]