We should probably focus on a single Mailing List for code reviews. This particular review is not necessarily specific to the rasterizer, so it should probably have only been sent to the 2d-dev list (Path2D is owned by 2D).

Also, please start a new thread for code reviews (and new topics). This code review will now be forever threaded with the long discussion about rasterizer optimizations because it shares too many headers with the other thread... :(

                        ...jim

On 3/6/15 10:35 AM, Laurent Bourgès wrote:
Jim,

Here is my first "official" webrev (vs graphics-rasterizer forest)
concerning Path2D copy constructors:
http://cr.openjdk.java.net/~lbourges/webrev_Path2D_0/

this is a simple Path2D patch to trim arrays (numTypes & float/double
Coords) in copy constructors (Path2D.Float and Path2D.Double variants)
with the requested test: Path2DTrimCopy.

Please tell me if the test is correct as I do not run it with jtreg
(annotations ?)


    2015-02-25 2:05 GMT+01:00 Jim Graham <james.gra...@oracle.com
    <mailto:james.gra...@oracle.com>>:

        Those changes were exactly what I was referring to.  I don't see
        why we shouldn't make trimmed arrays when copying the shape.
        I'm pretty sure that the copy constructors are going to be
        overwhelmingly used to make a protected copy of an existing
        shape/path2d which is likely meant mostly for reading.  In
        particular, in the case of the return value from
        createStrokedShape() I don't think the intention is to create
        the shape and then scribble on it, the intent is to treat the
        answer as if it were immutable - at least the 99.9% case - so I
        think a perfectly sized shape is OK.

        Be sure to add a test case that creates an empty Path2D, clones
        it, copy constructs it (to both .Double() and .Float() variants)
        and then tries to add new segments to it - to make sure that the
        array growth code doesn't get ArrayIndexOutOfBounds exceptions
        due to making assumptions about the lengths of the arrays (I
        eyeballed the makeRoom() code and it looks good, but we should
        test it if we are making arrays that are potentially zero length
        or very tiny)...


Laurent

Reply via email to