[jdk9] RFR: 8163517: Various cleanup in java.io code
Hello! Here's a cleanup pass through the code in java.io package. Would you please help review? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8163517 WEBREV: http://cr.openjdk.java.net/~igerasim/8163517/00/webrev/ With kind regards, Ivan
On 08/16/2016 03:53 PM, Ivan Gerasimov wrote:
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8163517 WEBREV: http://cr.openjdk.java.net/~igerasim/8163517/00/webrev/
Both ObjectInputStream and ObjectOutputStream can use auto-unboxing in these blocks? 1271 if (result.booleanValue()) { 1272 return; 1273 } ObjectStreamClass: I wonder if the entire getPackageName is replaceable with Class.getPackageName()? If so, ditch the method completely and use Class.getPackageName everywhere. This may complicate backporting to JDK 8 though. OutputStreamWriter: New code in append(CharSequence,int,int) unnecessarily calls toString on CharBuffers which are handled specially downstream in append(CharSequence). Thanks, -Aleksey
Thank you Aleksey for your careful review! On 16.08.2016 16:08, Aleksey Shipilev wrote:
On 08/16/2016 03:53 PM, Ivan Gerasimov wrote:
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8163517 WEBREV: http://cr.openjdk.java.net/~igerasim/8163517/00/webrev/ Both ObjectInputStream and ObjectOutputStream can use auto-unboxing in these blocks?
1271 if (result.booleanValue()) { 1272 return; 1273 }
Yes. I'll also invert the logic, as it reads better.
ObjectStreamClass:
I wonder if the entire getPackageName is replaceable with Class.getPackageName()? If so, ditch the method completely and use Class.getPackageName everywhere. This may complicate backporting to JDK 8 though.
Unfortunately, Class.getPackageName() cannot handle arrays, so I'll leave it as is.
OutputStreamWriter:
New code in append(CharSequence,int,int) unnecessarily calls toString on CharBuffers which are handled specially downstream in append(CharSequence).
Good catch, thanks! I'll replace the call to write(String) with append(CharSequence). This will make the code even shorter. Here is the updated webrev: http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/ With kind regards, Ivan.
On 2016-08-16 19:20, Ivan Gerasimov wrote:
ObjectStreamClass:
I wonder if the entire getPackageName is replaceable with Class.getPackageName()? If so, ditch the method completely and use Class.getPackageName everywhere. This may complicate backporting to JDK 8 though.
Unfortunately, Class.getPackageName() cannot handle arrays, so I'll leave it as is.
There are a number of utility methods around that could be reused, e.g., jdk.internal.reflect.Reflection.isSameClassPackage handles both arrays and primitive types and could replace both packageEquals and getPackageName in ObjectStreamClass (which already use j.i.r.Reflection for other things). Making isSameClassPackage in Reflection public should be non-controversial, I hope. Thanks! /Claes
Hi Claes! Thank you for looking into this. On 18.08.2016 15:25, Claes Redestad wrote:
On 2016-08-16 19:20, Ivan Gerasimov wrote:
ObjectStreamClass:
I wonder if the entire getPackageName is replaceable with Class.getPackageName()? If so, ditch the method completely and use Class.getPackageName everywhere. This may complicate backporting to JDK 8 though.
Unfortunately, Class.getPackageName() cannot handle arrays, so I'll leave it as is.
There are a number of utility methods around that could be reused, e.g., jdk.internal.reflect.Reflection.isSameClassPackage handles both arrays and primitive types and could replace both packageEquals and getPackageName in ObjectStreamClass (which already use j.i.r.Reflection for other things). Making isSameClassPackage in Reflection public should be non-controversial, I hope.
Yes, thanks for the pointer! That's right, there are several similar methods in different packages. I see that the third option is sun.invoke.util.VerifyAccess.isSamePackage / getPackageName, but it also has its nuances. I guess that keeping ObjectStreamClass.getPackageName() separated would be the simplest approach: it minimizes the chances of regression and doesn't make it harder to do future modifications to Reflection.isSameClassPackage, if it is ever needed. If there are no objections, I'd like to push the latest variant of the fix, which is at http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/ With kind regards, Ivan
On 2016-08-18 20:23, Ivan Gerasimov wrote:
Hi Claes!
Thank you for looking into this.
On 18.08.2016 15:25, Claes Redestad wrote:
On 2016-08-16 19:20, Ivan Gerasimov wrote:
ObjectStreamClass:
I wonder if the entire getPackageName is replaceable with Class.getPackageName()? If so, ditch the method completely and use Class.getPackageName everywhere. This may complicate backporting to JDK 8 though.
Unfortunately, Class.getPackageName() cannot handle arrays, so I'll leave it as is.
There are a number of utility methods around that could be reused, e.g., jdk.internal.reflect.Reflection.isSameClassPackage handles both arrays and primitive types and could replace both packageEquals and getPackageName in ObjectStreamClass (which already use j.i.r.Reflection for other things). Making isSameClassPackage in Reflection public should be non-controversial, I hope.
Yes, thanks for the pointer! That's right, there are several similar methods in different packages. I see that the third option is sun.invoke.util.VerifyAccess.isSamePackage / getPackageName, but it also has its nuances.
I guess that keeping ObjectStreamClass.getPackageName() separated would be the simplest approach: it minimizes the chances of regression and doesn't make it harder to do future modifications to Reflection.isSameClassPackage, if it is ever needed.
If there are no objections, I'd like to push the latest variant of the fix, which is at http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/
Go ahead, I'm just musing about the opportunity to consolidate further, which is better kept as a follow-up anyhow. Rather than breaking into Reflection we should likely add an internally public API with a more well-defined contract, ensuring that it addresses arrays, primitives etc, and then Reflection, VerifyAccess and ObjectStream could consolidate their use to such a method. Thanks! /Claes
On 18.08.2016 21:27, Claes Redestad wrote:
Rather than breaking into Reflection we should likely add an internally public API with a more well-defined contract, ensuring that it addresses arrays, primitives etc, and then Reflection, VerifyAccess and ObjectStream could consolidate their use to such a method.
Yes, totally agree with this.
On 08/16/2016 08:20 PM, Ivan Gerasimov wrote:
Here is the updated webrev: http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/
Looks good to me. -Aleksey
Hi Ivan, Look fine. Remember to use the enhancement process[1] to get approval for this enhancement. Roger [1] http://openjdk.java.net/projects/jdk9/fc-extension-process On 8/16/2016 8:53 AM, Ivan Gerasimov wrote:
Hello!
Here's a cleanup pass through the code in java.io package.
Would you please help review?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8163517 WEBREV: http://cr.openjdk.java.net/~igerasim/8163517/00/webrev/
With kind regards, Ivan
Thanks Roger for the review and the pointer! I'll follow the process. With kind regards, Ivan On 16.08.2016 17:30, Roger Riggs wrote:
Hi Ivan,
Look fine.
Remember to use the enhancement process[1] to get approval for this enhancement.
Roger
[1] http://openjdk.java.net/projects/jdk9/fc-extension-process
On 8/16/2016 8:53 AM, Ivan Gerasimov wrote:
Hello!
Here's a cleanup pass through the code in java.io package.
Would you please help review?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8163517 WEBREV: http://cr.openjdk.java.net/~igerasim/8163517/00/webrev/
With kind regards, Ivan
participants (4)
-
Aleksey Shipilev
-
Claes Redestad
-
Ivan Gerasimov
-
Roger Riggs