[10] RFR 8078192: Path2D storage trimming

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/


Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Prahalad kumar Narayanan
Hello Laurent

The code changes look good.
    . The new method trimToSize effectively does the same operation (Arrays.copyOf) as done by the copy contructors.
    . The test file- Path2DCopyConstructor now tests after executing the methods pathObj.trimToSize and pathObj.clone

With regard to the text in Javadoc comments
    . I felt, the first line clearly explains the operation -
               "Trims the capacity of this Path2D instance to its current size."
    . The immediate following line is redundant and may not be required -
               "If the capacity .. is larger than its current size... "
    . The third line will be required till the end of the comments -
               "An application can use ... minimize the storage of a path. ... since 10"
    . This is my observation. You could wait for other suggestions.

Btw, the copyright year should be changed to 2017 in both Path2D.java and the test file.
(Some use automation scripts that modify the year at the time of check-in. If so, kindly ignore the observation)

Thanks
Have a good day

Prahalad N.


----------------------------------------------------------------------

Message: 1
Date: Wed, 19 Apr 2017 08:49:33 +0200
From: Laurent Bourg?s <[hidden email]>
To: "[hidden email]" <[hidden email]>, Phil Race
        <[hidden email]>,  Jim Graham <[hidden email]>, Iris
        Clark <[hidden email]>
Subject: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming
Message-ID:
        <CAKjRUT7zQ=[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
I fixed the copyright dates and the javadoc as you proposed (I initially derived the javadoc from Vector.trimToSize)

Thanks,
Laurent

2017-04-20 5:46 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Hello Laurent

The code changes look good.
    . The new method trimToSize effectively does the same operation (Arrays.copyOf) as done by the copy contructors.
    . The test file- Path2DCopyConstructor now tests after executing the methods pathObj.trimToSize and pathObj.clone

With regard to the text in Javadoc comments
    . I felt, the first line clearly explains the operation -
               "Trims the capacity of this Path2D instance to its current size."
    . The immediate following line is redundant and may not be required -
               "If the capacity .. is larger than its current size... "
    . The third line will be required till the end of the comments -
               "An application can use ... minimize the storage of a path. ... since 10"
    . This is my observation. You could wait for other suggestions.

Btw, the copyright year should be changed to 2017 in both Path2D.java and the test file.
(Some use automation scripts that modify the year at the time of check-in. If so, kindly ignore the observation)

Thanks
Have a good day

Prahalad N.


----------------------------------------------------------------------

Message: 1
Date: Wed, 19 Apr 2017 08:49:33 +0200
From: Laurent Bourg?s <[hidden email]>
To: "[hidden email]" <[hidden email]>, Phil Race
        <[hidden email]>,  Jim Graham <[hidden email]>, Iris
        Clark <[hidden email]>
Subject: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming
Message-ID:
        <CAKjRUT7zQ=[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Prahalad kumar Narayanan
Looks good to me.

Thanks
Have a good day

Prahalad N.

----------------------------------------------------------------------

From: Laurent Bourgès [mailto:[hidden email]]
Sent: Thursday, April 20, 2017 11:51 AM
To: Prahalad Kumar Narayanan
Cc: [hidden email]
Subject: Re: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming

Hello,
Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.1/
I fixed the copyright dates and the javadoc as you proposed (I initially derived the javadoc from Vector.trimToSize)
Thanks,
Laurent

2017-04-20 5:46 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Hello Laurent

The code changes look good.
    . The new method trimToSize effectively does the same operation (Arrays.copyOf) as done by the copy contructors.
    . The test file- Path2DCopyConstructor now tests after executing the methods pathObj.trimToSize and pathObj.clone

With regard to the text in Javadoc comments
    . I felt, the first line clearly explains the operation -
               "Trims the capacity of this Path2D instance to its current size."
    . The immediate following line is redundant and may not be required -
               "If the capacity .. is larger than its current size... "
    . The third line will be required till the end of the comments -
               "An application can use ... minimize the storage of a path. ... since 10"
    . This is my observation. You could wait for other suggestions.

Btw, the copyright year should be changed to 2017 in both Path2D.java and the test file.
(Some use automation scripts that modify the year at the time of check-in. If so, kindly ignore the observation)

Thanks
Have a good day

Prahalad N.


----------------------------------------------------------------------

Message: 1
Date: Wed, 19 Apr 2017 08:49:33 +0200
From: Laurent Bourg?s <[hidden email]>
To: "[hidden email]" <[hidden email]>, Phil Race
        <[hidden email]>,  Jim Graham <[hidden email]>, Iris
        Clark <[hidden email]>
Subject: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming
Message-ID:
        <CAKjRUT7zQ=[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Phil,

Do you approve the patch and the javadoc too ?

If yes, could you push it for me into OpenJDK10 ?

Laurent


2017-04-20 11:55 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Looks good to me.

Thanks
Have a good day

Prahalad N.

----------------------------------------------------------------------

From: Laurent Bourgès [mailto:[hidden email]]
Sent: Thursday, April 20, 2017 11:51 AM
To: Prahalad Kumar Narayanan
Cc: [hidden email]
Subject: Re: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming

Hello,
Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.1/
I fixed the copyright dates and the javadoc as you proposed (I initially derived the javadoc from Vector.trimToSize)
Thanks,
Laurent

2017-04-20 5:46 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Hello Laurent

The code changes look good.
    . The new method trimToSize effectively does the same operation (Arrays.copyOf) as done by the copy contructors.
    . The test file- Path2DCopyConstructor now tests after executing the methods pathObj.trimToSize and pathObj.clone

With regard to the text in Javadoc comments
    . I felt, the first line clearly explains the operation -
               "Trims the capacity of this Path2D instance to its current size."
    . The immediate following line is redundant and may not be required -
               "If the capacity .. is larger than its current size... "
    . The third line will be required till the end of the comments -
               "An application can use ... minimize the storage of a path. ... since 10"
    . This is my observation. You could wait for other suggestions.

Btw, the copyright year should be changed to 2017 in both Path2D.java and the test file.
(Some use automation scripts that modify the year at the time of check-in. If so, kindly ignore the observation)

Thanks
Have a good day

Prahalad N.


----------------------------------------------------------------------

Message: 1
Date: Wed, 19 Apr 2017 08:49:33 +0200
From: Laurent Bourg?s <[hidden email]>
To: "[hidden email]" <[hidden email]>, Phil Race
        <[hidden email]>,  Jim Graham <[hidden email]>, Iris
        Clark <[hidden email]>
Subject: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming
Message-ID:
        <CAKjRUT7zQ=[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Philip Race
Hi,

This can't be pushed without a CSR approval for JDK10 and that process won't
be "up and running" for about another 10 days.

But before we get to that I wonder why we need to return Path2D when we
aren't creating a new one .. there doesn't seem to be any value to it and
it just potentially misleading a caller into thinking it has the cost of doing so.

If any value is worth returning it might be a boolean to indicate whether any trimming
actually occurred.

-phil.

On 4/20/17, 4:41 AM, Laurent Bourgès wrote:
Phil,

Do you approve the patch and the javadoc too ?

If yes, could you push it for me into OpenJDK10 ?

Laurent


2017-04-20 11:55 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Looks good to me.

Thanks
Have a good day

Prahalad N.

----------------------------------------------------------------------

From: Laurent Bourgès [mailto:[hidden email]]
Sent: Thursday, April 20, 2017 11:51 AM
To: Prahalad Kumar Narayanan
Cc: [hidden email]
Subject: Re: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming

Hello,
Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.1/
I fixed the copyright dates and the javadoc as you proposed (I initially derived the javadoc from Vector.trimToSize)
Thanks,
Laurent

2017-04-20 5:46 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Hello Laurent

The code changes look good.
    . The new method trimToSize effectively does the same operation (Arrays.copyOf) as done by the copy contructors.
    . The test file- Path2DCopyConstructor now tests after executing the methods pathObj.trimToSize and pathObj.clone

With regard to the text in Javadoc comments
    . I felt, the first line clearly explains the operation -
               "Trims the capacity of this Path2D instance to its current size."
    . The immediate following line is redundant and may not be required -
               "If the capacity .. is larger than its current size... "
    . The third line will be required till the end of the comments -
               "An application can use ... minimize the storage of a path. ... since 10"
    . This is my observation. You could wait for other suggestions.

Btw, the copyright year should be changed to 2017 in both Path2D.java and the test file.
(Some use automation scripts that modify the year at the time of check-in. If so, kindly ignore the observation)

Thanks
Have a good day

Prahalad N.


----------------------------------------------------------------------

Message: 1
Date: Wed, 19 Apr 2017 08:49:33 +0200
From: Laurent Bourg?s <[hidden email]>
To: "[hidden email]" <[hidden email]>, Phil Race
        <[hidden email]>,  Jim Graham <[hidden email]>, Iris
        Clark <[hidden email]>
Subject: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming
Message-ID:
        <CAKjRUT7zQ=[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Hi Phil,

This can't be pushed without a CSR approval for JDK10 and that process won't
be "up and running" for about another 10 days.

Ok, no problem. As the CCC was approved for JDK9, I wondered if that was needed again.
 

But before we get to that I wonder why we need to return Path2D when we
aren't creating a new one .. there doesn't seem to be any value to it and
it just potentially misleading a caller into thinking it has the cost of doing so.

I agree, it only provides a fluent interface and mimics the clone() approach, but you're right, it is not really needed.
I will remove that and fix the test that uses that method:
        testAddLine(pf.trimToSize(), isEmpty);

Laurent


If any value is worth returning it might be a boolean to indicate whether any trimming
actually occurred.

-phil.


On 4/20/17, 4:41 AM, Laurent Bourgès wrote:
Phil,

Do you approve the patch and the javadoc too ?

If yes, could you push it for me into OpenJDK10 ?

Laurent


2017-04-20 11:55 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Looks good to me.

Thanks
Have a good day

Prahalad N.

----------------------------------------------------------------------

From: Laurent Bourgès [mailto:[hidden email]]
Sent: Thursday, April 20, 2017 11:51 AM
To: Prahalad Kumar Narayanan
Cc: [hidden email]
Subject: Re: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming

Hello,
Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.1/
I fixed the copyright dates and the javadoc as you proposed (I initially derived the javadoc from Vector.trimToSize)
Thanks,
Laurent

2017-04-20 5:46 GMT+02:00 Prahalad Kumar Narayanan <[hidden email]>:
Hello Laurent

The code changes look good.
    . The new method trimToSize effectively does the same operation (Arrays.copyOf) as done by the copy contructors.
    . The test file- Path2DCopyConstructor now tests after executing the methods pathObj.trimToSize and pathObj.clone

With regard to the text in Javadoc comments
    . I felt, the first line clearly explains the operation -
               "Trims the capacity of this Path2D instance to its current size."
    . The immediate following line is redundant and may not be required -
               "If the capacity .. is larger than its current size... "
    . The third line will be required till the end of the comments -
               "An application can use ... minimize the storage of a path. ... since 10"
    . This is my observation. You could wait for other suggestions.

Btw, the copyright year should be changed to 2017 in both Path2D.java and the test file.
(Some use automation scripts that modify the year at the time of check-in. If so, kindly ignore the observation)

Thanks
Have a good day

Prahalad N.


----------------------------------------------------------------------

Message: 1
Date: Wed, 19 Apr 2017 08:49:33 +0200
From: Laurent Bourg?s <[hidden email]>
To: "[hidden email]" <[hidden email]>, Phil Race
        <[hidden email]>,  Jim Graham <[hidden email]>, Iris
        Clark <[hidden email]>
Subject: [OpenJDK 2D-Dev] [10] RFR 8078192: Path2D storage trimming
Message-ID:
        <CAKjRUT7zQ=[hidden email]>
Content-Type: text/plain; charset="utf-8"

Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Jim Graham-5
In reply to this post by Laurent Bourgès
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar
trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining
paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim

On 4/18/17 11:49 PM, Laurent Bourgès wrote:

> Hi,
>
> Here is a first attempt to propose a Path2D patch (based on JDK10):
> http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8078192
>
> Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
> all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()
>
> Cheers,
> Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Philip Race
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.

On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Philip Race
OK. Although we still need to wait for the CSR process.

-phil.

On 4/20/17, 3:05 PM, Laurent Bourgès wrote:
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Phil,

Did you get any answer from the CSR process on this bug ?

Laurent


2017-04-21 0:17 GMT+02:00 Philip Race <[hidden email]>:
OK. Although we still need to wait for the CSR process.

-phil.


On 4/20/17, 3:05 PM, Laurent Bourgès wrote:
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Philip Race
Early next week is the hope.

-phil

On 05/16/2017 02:20 PM, Laurent Bourgès wrote:
Phil,

Did you get any answer from the CSR process on this bug ?

Laurent


2017-04-21 0:17 GMT+02:00 Philip Race <[hidden email]>:
OK. Although we still need to wait for the CSR process.

-phil.


On 4/20/17, 3:05 PM, Laurent Bourgès wrote:
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Phil,

Could you initiate the CSR request in JBS ? 
I do not see how to create it from the current bug: I looked at the More menu but I do not have any Create CSR action.

Cheers,
Laurent

Le 17 mai 2017 22:05, "Phil Race" <[hidden email]> a écrit :
Early next week is the hope.

-phil

On 05/16/2017 02:20 PM, Laurent Bourgès wrote:
Phil,

Did you get any answer from the CSR process on this bug ?

Laurent


2017-04-21 0:17 GMT+02:00 Philip Race <[hidden email]>:
OK. Although we still need to wait for the CSR process.

-phil.


On 4/20/17, 3:05 PM, Laurent Bourgès wrote:
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Philip Race
Oh this one is interesting. It already has a CSR :-)



That's because we had created a CCC for it back in JDK 9 and then withdrew it.

All existing CSRs were ported over but as "confidential" since honestly
no one had time to look at all N thousand of them and vet them ...
we vetted and opened enough to give people a flavour.

So I think that "create a csr" doesn't appear if there already is one
since you can't see it.

I opened it so you can now see it.

https://bugs.openjdk.java.net/browse/CCC-8078192

I re-opened it but don't seem to be able to target it to 10 or assign it to you.

I'll need to ask.

-phil

On 6/7/17, 1:35 PM, Laurent Bourgès wrote:
Phil,

Could you initiate the CSR request in JBS ? 
I do not see how to create it from the current bug: I looked at the More menu but I do not have any Create CSR action.

Cheers,
Laurent

Le 17 mai 2017 22:05, "Phil Race" <[hidden email]> a écrit :
Early next week is the hope.

-phil

On 05/16/2017 02:20 PM, Laurent Bourgès wrote:
Phil,

Did you get any answer from the CSR process on this bug ?

Laurent


2017-04-21 0:17 GMT+02:00 Philip Race <[hidden email]>:
OK. Although we still need to wait for the CSR process.

-phil.


On 4/20/17, 3:05 PM, Laurent Bourgès wrote:
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Philip Race
Laurent,

Investigation has determined that since this was ported over in the "CCC migration project",
due to the way that is set up it can't be assigned to non-Oracle employees.

It won't affect any new CSRs that are created.

Since the likelihood of any other similar case is extremely low, the simplest answer is
that either Jim or myself will need to own and edit this one on your behalf.

Send Jim or myself the changes you would like to make off-line and we'll take care of it.

-phil.


On 06/07/2017 01:56 PM, Philip Race wrote:
Oh this one is interesting. It already has a CSR :-)



That's because we had created a CCC for it back in JDK 9 and then withdrew it.

All existing CSRs were ported over but as "confidential" since honestly
no one had time to look at all N thousand of them and vet them ...
we vetted and opened enough to give people a flavour.

So I think that "create a csr" doesn't appear if there already is one
since you can't see it.

I opened it so you can now see it.

https://bugs.openjdk.java.net/browse/CCC-8078192

I re-opened it but don't seem to be able to target it to 10 or assign it to you.

I'll need to ask.

-phil

On 6/7/17, 1:35 PM, Laurent Bourgès wrote:
Phil,

Could you initiate the CSR request in JBS ? 
I do not see how to create it from the current bug: I looked at the More menu but I do not have any Create CSR action.

Cheers,
Laurent

Le 17 mai 2017 22:05, "Phil Race" <[hidden email]> a écrit :
Early next week is the hope.

-phil

On 05/16/2017 02:20 PM, Laurent Bourgès wrote:
Phil,

Did you get any answer from the CSR process on this bug ?

Laurent


2017-04-21 0:17 GMT+02:00 Philip Race <[hidden email]>:
OK. Although we still need to wait for the CSR process.

-phil.


On 4/20/17, 3:05 PM, Laurent Bourgès wrote:
Sorry (bad shortcut);

Here is the fixed webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.3/

Laurent

2017-04-21 0:04 GMT+02:00 Laurent Bourgès <[hidden email]>:
Sorry for the typo, I added also a newline before @since:


2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor test which checks
all public Path2D methods on concrete classes (Path2D.Float, Path2D.Double, GeneralPath) after calling path.trimToSize()

Cheers,
Laurent



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès



--
--
Laurent Bourgès


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
Phil,

Here are my comments on this CSR to be fixed in openjdk 10; I already submitted a patch and the review process happened in may.

Le 3 juil. 2017 7:41 PM, "Phil Race" <[hidden email]> a écrit :
Laurent,

Investigation has determined that since this was ported over in the "CCC migration project",
due to the way that is set up it can't be assigned to non-Oracle employees.

Ok, no problem.


It won't affect any new CSRs that are created.

Since the likelihood of any other similar case is extremely low, the simplest answer is
that either Jim or myself will need to own and edit this one on your behalf.

Send Jim or myself the changes you would like to make off-line and we'll take care of it.

Just change the fix version to 10 and the @since javadoc tag as below:

  /** 
    * Trims the capacity of this Path2D instance to its current 
     * size. An application can use this operation to minimize the 
     * storage of a path. 
     * 
     * @since 10 
     */ +    public abstract void trimToSize(); 

It will match the proposed patch.

Thanks,
Laurent



2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR 8078192: Path2D storage trimming

Laurent Bourgès
In reply to this post by Philip Race
Phil,

Here are my comments on this CSR to be fixed in openjdk 10; I already submitted a patch and the review process happened in may.

Le 3 juil. 2017 7:41 PM, "Phil Race" <[hidden email]> a écrit :
Laurent,

Investigation has determined that since this was ported over in the "CCC migration project",
due to the way that is set up it can't be assigned to non-Oracle employees.

Ok, no problem.


It won't affect any new CSRs that are created.

Since the likelihood of any other similar case is extremely low, the simplest answer is
that either Jim or myself will need to own and edit this one on your behalf.

Send Jim or myself the changes you would like to make off-line and we'll take care of it.

Just change the fix version to 10 and the @since javadoc tag as below:

  /** 
    * Trims the capacity of this Path2D instance to its current 
     * size. An application can use this operation to minimize the 
     * storage of a path. 
     * 
     * @since 10 
     */ +    public abstract void trimToSize(); 

It will match the proposed patch.

Thanks,
Laurent



2017-04-20 23:04 GMT+02:00 Philip Race <[hidden email]>:
 You have a capital letter here and I think it must be lower case ..


2499      * @Since 10

-phil.


On 4/20/17, 1:58 PM, Laurent Bourgès wrote:
Hi Phil & Jim,

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.2/

Changes:
- trimToSize() return void
- fixed test + jtreg passed

Bye,
Laurent

2017-04-20 21:30 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

The implementation looks good, except that the method chaining-style return value seems out of place here.  Similar trimToSize() methods in Collections return void and none of the other methods in this area use the method chaining paradigm.  In the interest of maintaining a common design theme throughout 2D this method should just return void.

                        ...jim


On 4/18/17 11:49 PM, Laurent Bourgès wrote:
Hi,

Here is a first attempt to propose a Path2D patch (based on JDK10):
http://cr.openjdk.java.net/~lbourges/path2D/Path2D-8078192.0/

JBS: https://bugs.openjdk.java.net/browse/JDK-8078192

Please review the Path2D changes, notably the javadoc (english) and the modified Path2DCopyConstructor
12
Loading...