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

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [OpenJDK Rasterizer] Openjdk java2d rasterizer JEP for pisces (marlin) enhancem
From:       Jim Graham <james.graham () oracle ! com>
Date:       2015-03-07 1:45:29
Message-ID: 54FA5839.9030200 () oracle ! com
[Download RAW message or body]

On lines 228 and 1069 you should not mix p2d and this.  I would go with 
both p2d.pointTypes and p2d.numTypes in both cases...

			...jim

On 3/6/15 2:03 PM, Laurent Bourgès wrote:
> Phil,
>
> Here is a new webrev:
> http://cr.openjdk.java.net/~lbourges/webrev_Path2D_1/
>
> See my comments below:
>
>>         you placed the test in the java.awt.geom package.
>>
>>           25 package java.awt.geom;
>>
>>         and are accessing internals of that package.
>>
>>         In jigsaw/modular mode that won't even compile.
>>
>>
>>     Ok it is annoying:
>>     as all Path2D fields are package protected, I designed the test
>>     using direct access to any fields ...
>>
>>
>>         So the test should go in the anonymous package and avoid
>>         accessing internals.
>>         It should be possible to use just public API  to verify the
>>         arrays  of a shape
>>         being cloned are trimmed .
>>
>>
>>     No, it is not possible to use Shape API to access arrays nor
>>     fields (numTypes ...):
>>     only getPathIterator() could give me data but it won't tell me if
>>     the underlying arrays or fields are correct.
>
>     That is true ..
>
>     Well, if you need it to be in java.awt.geom, I think even today
>     you'll find it won't work
>     unless you jump through some jtreg hoops to install it on the
>     bootclasspath.
>     I think its something like "@run main/othervm -Xbootclasspath/a:. "
>     And later in the modular JDK it will need to be modified again.
>
>     I'd say either update the test to work with jtreg today - and test
>     it to be sure,
>     or provide the test without an @test tag, or with an @ignore tag, so
>     people can
>     still manually verify it but the harness won't run it.
>
>
> I removed the @test tag but added comments indicating to run the test
> manually.
>
>
>>
>>     Maybe I could use introspection to getDeclaredField(name) and
>>     setAccessible(true) to get internal data.
>
>     That won't work either. So maybe this is a "noreg-hard" or
>     "noreg-cleanup" bug.
>     We add those labels to the JBS/JIRA bug when something isn't testable.
>
>
> Nevermind.
>
>
>>     Any idea or utility class I could use
>
>>
>>         Why is it necessary to explicitly add the call to super(); ?
>>
>>         223             super();
>>
>>
>>     I agree it is not necessary but it explicitely says that I use the
>>     empty constructor:
>>
>>         /**
>>          * Constructs a new empty {@code Path2D} object.
>>          * It is assumed that the package sibling subclass that is
>>          * defaulting to this constructor will fill in all values.
>>          *
>>          * @since 1.6
>>          */
>>         /* private protected */
>>         Path2D() {
>>         }
>
>     If we all did this, all of the time, there'd be a lot of extra lines
>     in the code, that the compiler
>     would fill in for us anyway.
>
>
> I removed the superfluous super() calls.
>
> Thanks for your review,
>
> Laurent
[prev in list] [next in list] [prev in thread] [next in thread]