Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
Hi Christoph, On Wed, Jun 28, 2017 at 4:47 PM, Langer, Christophwrote: > Hi Thomas, > > thanks for your review! > > > com/sun/tools/jdi/InvokableTypeImpl.java: > > > > import com.sun.jdi.VMCannotBeModifiedException; ?Why do we need to > > import this type, we do not seem to use it? > > This is needed for the documentation, see line 97, the throws > documentation. > > > ObjectReferenceImpl: > > > > the removed code: Looks like part of the checks are missing since a long > time. > > The remains could be understood as a check that the class for the method > to > > be invoked exists in the debuggee. But then, there are no guarantees > > anyway that inbetween firing the invoke command to the debuggee and the > > debuggee processing the invoke command that class may not be unloaded, > > so we may just rely on the jdwp invoke command failing for that case. > So, ok > > to remove it IMHO. > > I thought so, too. > > > SDE.java: > > > > @SuppressWarnings("unused") ? which member is unused? > > It's about member "njplsEnd". Still it looks cleaner not to remove it but > to suppress the warning. > > > "SocketTransportService.java: pull out SocketConnection to an own file > > SocketConnection.java" - why? > > In the codebase that I was merging the package with, the class > SocketConnection needed to be declared public for some reason. > This is only allowed if the code lives in a file which is named like the > class. And since I think it's generally not wrong to have classes > in their own file and I find other package private classes of > com.sun.tools.jdi their own class files, too, I thought it's a win-win to > pull it out. :) > It will ease future merging. > > > com/sun/tools/jdi/VMModifiers.java: > > > > Not touched by your change, but weird: In VMModifiers, some of the > > constants share numerical values: > > > > 28 public interface VMModifiers { > > ... > > 35 int VOLATILE = 0x0040; /* can cache in registers */ > > 36 int BRIDGE = 0x0040;/* Bridge method generated by > compiler > > */ > > > > 37 int TRANSIENT = 0x0080; /* not persistant */ > > 38 int VARARGS = 0x0080; /* Method accepts var. args*/ > > ... > > > > So, VOLATILE == BRIDGE and TRANSIENT == VARARGS? This seems wrong, no? > > This really looks weird, I agree. But it's out of scope for me to dig > further... :) > > As I addressed all the points you mentioned, I will consider the change > reviewed by you. > > Best regards > Christoph > > Thanks for the answers, and I am fine with the change, no need for a new webrev. Kind Regards, Thomas
RE: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
Hi Thomas, thanks for your review! > com/sun/tools/jdi/InvokableTypeImpl.java: > > import com.sun.jdi.VMCannotBeModifiedException; ?Why do we need to > import this type, we do not seem to use it? This is needed for the documentation, see line 97, the throws documentation. > ObjectReferenceImpl: > > the removed code: Looks like part of the checks are missing since a long time. > The remains could be understood as a check that the class for the method to > be invoked exists in the debuggee. But then, there are no guarantees > anyway that inbetween firing the invoke command to the debuggee and the > debuggee processing the invoke command that class may not be unloaded, > so we may just rely on the jdwp invoke command failing for that case. So, ok > to remove it IMHO. I thought so, too. > SDE.java: > > @SuppressWarnings("unused") ? which member is unused? It's about member "njplsEnd". Still it looks cleaner not to remove it but to suppress the warning. > "SocketTransportService.java: pull out SocketConnection to an own file > SocketConnection.java" - why? In the codebase that I was merging the package with, the class SocketConnection needed to be declared public for some reason. This is only allowed if the code lives in a file which is named like the class. And since I think it's generally not wrong to have classes in their own file and I find other package private classes of com.sun.tools.jdi their own class files, too, I thought it's a win-win to pull it out. :) It will ease future merging. > com/sun/tools/jdi/VMModifiers.java: > > Not touched by your change, but weird: In VMModifiers, some of the > constants share numerical values: > > 28 public interface VMModifiers { > ... > 35 int VOLATILE = 0x0040; /* can cache in registers */ > 36 int BRIDGE = 0x0040; /* Bridge method generated by > compiler > */ > > 37 int TRANSIENT = 0x0080; /* not persistant */ > 38 int VARARGS = 0x0080; /* Method accepts var. args*/ > ... > > So, VOLATILE == BRIDGE and TRANSIENT == VARARGS? This seems wrong, no? This really looks weird, I agree. But it's out of scope for me to dig further... :) As I addressed all the points you mentioned, I will consider the change reviewed by you. Best regards Christoph
Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
Hi Christoph, Thanks for the cleanup! Here some minor remarks (like Serguei, I did not check the expanded imports). --- com/sun/tools/jdi/InvokableTypeImpl.java: import com.sun.jdi.VMCannotBeModifiedException; ?Why do we need to import this type, we do not seem to use it? --- ObjectReferenceImpl: the removed code: Looks like part of the checks are missing since a long time. The remains could be understood as a check that the class for the method to be invoked exists in the debuggee. But then, there are no guarantees anyway that inbetween firing the invoke command to the debuggee and the debuggee processing the invoke command that class may not be unloaded, so we may just rely on the jdwp invoke command failing for that case. So, ok to remove it IMHO. --- SDE.java: @SuppressWarnings("unused") ? which member is unused? --- "SocketTransportService.java: pull out SocketConnection to an own file SocketConnection.java" - why? --- com/sun/tools/jdi/VMModifiers.java: Not touched by your change, but weird: In VMModifiers, some of the constants share numerical values: 28 public interface VMModifiers { ... 35 int VOLATILE = 0x0040; /* can cache in registers */ 36 int BRIDGE = 0x0040;/* Bridge method generated by compiler */ 37 int TRANSIENT = 0x0080; /* not persistant */ 38 int VARARGS = 0x0080; /* Method accepts var. args*/ ... So, VOLATILE == BRIDGE and TRANSIENT == VARARGS? This seems wrong, no? Kind Regards, Thomas On Tue, Jun 27, 2017 at 9:09 PM, Langer, Christophwrote: > Hi, > > > > I’ve got another contribution for cleaning up the jdk.jdi classes. This > time it’s about package com.sun.tools.jdi. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8183012 > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183012.0/ > > > > What I’ve done: > > I’ve largely cleaned up the import statements (removing obsolete imports, > expanding “*” wildcards, sorting). Furthermore I did quite a few whitespace > cleanups to make the code look nicer. And I removed the types in > constructors of typed classes. > > > > The more interesting stuff which should probably closely be reviewed is > the following: > > a) Remove unnecessary casts in BooleanValueImpl.java, ByteValueImpl.java, > CharValueImpl.java, FloatValueImpl.java, LongValueImpl.java, > PacketStream.java, ShortValueImpl.java > > b) Remove redundant super interfaces in the class declarations of > ClassLoaderReferenceImpl.java, ConnectorImpl.java, > RawCommandLineLauncher.java, SunCommandLineLauncher.java, > ThreadGroupReferenceImpl.java, ThreadReferenceImpl.java > > c) ObjectReferenceImpl.java: Remove some code in void > validateClassMethodInvocation(Method method, int options). Some very old > comment suggests that the code was still there because nobody divined what > it was useful for. But I couldn’t find anything that seems to be really > useful here, except if the caller maybe wants to get some exception if such > occurred in the course of resolving the class or something like that. > > d) SocketTransportService.java: pull out SocketConnection to an own file > SocketConnection.java, pull class SocketTransportServiceCapabilities into > anonymous class within SocketTransportService.capabilities() > > e) RawCommandLineLauncher.java, SunCommandLineLauncher.java: Modifications > to constructing the TransportService object in its constructors > > > > Module jdk.jdi still builds without warnings after my change and I’m > currently running the jdi jtreg suite. > > > > Thanks in advance and best regards > > Christoph > > >
Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
On 6/28/17 01:43, Langer, Christoph wrote: Hi Serguei, thanks for the review. The reordering of the imports was done by my Eclipse tooling. And as it builds (and also jtreg did not show a regression (on Windows)), I would suspect it is ok. Yes, I'm thinking about the same. Thank you for the confirmation. -Serguei Best regards Christoph From: serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com] Sent: Mittwoch, 28. Juni 2017 10:04 To: Langer, Christoph <christoph.lan...@sap.com>; serviceability-dev@openjdk.java.net Subject: Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi Hi Christoph, It looks good to me. I do not have time to check the imports and their sorting. Thanks a lot for the cleanup! Serguei On 6/27/17 12:09, Langer, Christoph wrote: Hi, I’ve got another contribution for cleaning up the jdk.jdi classes. This time it’s about package com.sun.tools.jdi. Bug: https://bugs.openjdk.java.net/browse/JDK-8183012 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183012.0/ What I’ve done: I’ve largely cleaned up the import statements (removing obsolete imports, expanding “*” wildcards, sorting). Furthermore I did quite a few whitespace cleanups to make the code look nicer. And I removed the types in constructors of typed classes. The more interesting stuff which should probably closely be reviewed is the following: a) Remove unnecessary casts in BooleanValueImpl.java, ByteValueImpl.java, CharValueImpl.java, FloatValueImpl.java, LongValueImpl.java, PacketStream.java, ShortValueImpl.java b) Remove redundant super interfaces in the class declarations of ClassLoaderReferenceImpl.java, ConnectorImpl.java, RawCommandLineLauncher.java, SunCommandLineLauncher.java, ThreadGroupReferenceImpl.java, ThreadReferenceImpl.java c) ObjectReferenceImpl.java: Remove some code in void validateClassMethodInvocation(Method method, int options). Some very old comment suggests that the code was still there because nobody divined what it was useful for. But I couldn’t find anything that seems to be really useful here, except if the caller maybe wants to get some exception if such occurred in the course of resolving the class or something like that. d) SocketTransportService.java: pull out SocketConnection to an own file SocketConnection.java, pull class SocketTransportServiceCapabilities into anonymous class within SocketTransportService.capabilities() e) RawCommandLineLauncher.java, SunCommandLineLauncher.java: Modifications to constructing the TransportService object in its constructors Module jdk.jdi still builds without warnings after my change and I’m currently running the jdi jtreg suite. Thanks in advance and best regards Christoph
RE: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
Hi Serguei, thanks for the review. The reordering of the imports was done by my Eclipse tooling. And as it builds (and also jtreg did not show a regression (on Windows)), I would suspect it is ok. Best regards Christoph From: serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com] Sent: Mittwoch, 28. Juni 2017 10:04 To: Langer, Christoph <christoph.lan...@sap.com>; serviceability-dev@openjdk.java.net Subject: Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi Hi Christoph, It looks good to me. I do not have time to check the imports and their sorting. Thanks a lot for the cleanup! Serguei On 6/27/17 12:09, Langer, Christoph wrote: Hi, I’ve got another contribution for cleaning up the jdk.jdi classes. This time it’s about package com.sun.tools.jdi. Bug: https://bugs.openjdk.java.net/browse/JDK-8183012 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183012.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8183012.0/> What I’ve done: I’ve largely cleaned up the import statements (removing obsolete imports, expanding “*” wildcards, sorting). Furthermore I did quite a few whitespace cleanups to make the code look nicer. And I removed the types in constructors of typed classes. The more interesting stuff which should probably closely be reviewed is the following: a) Remove unnecessary casts in BooleanValueImpl.java, ByteValueImpl.java, CharValueImpl.java, FloatValueImpl.java, LongValueImpl.java, PacketStream.java, ShortValueImpl.java b) Remove redundant super interfaces in the class declarations of ClassLoaderReferenceImpl.java, ConnectorImpl.java, RawCommandLineLauncher.java, SunCommandLineLauncher.java, ThreadGroupReferenceImpl.java, ThreadReferenceImpl.java c) ObjectReferenceImpl.java: Remove some code in void validateClassMethodInvocation(Method method, int options). Some very old comment suggests that the code was still there because nobody divined what it was useful for. But I couldn’t find anything that seems to be really useful here, except if the caller maybe wants to get some exception if such occurred in the course of resolving the class or something like that. d) SocketTransportService.java: pull out SocketConnection to an own file SocketConnection.java, pull class SocketTransportServiceCapabilities into anonymous class within SocketTransportService.capabilities() e) RawCommandLineLauncher.java, SunCommandLineLauncher.java: Modifications to constructing the TransportService object in its constructors Module jdk.jdi still builds without warnings after my change and I’m currently running the jdi jtreg suite. Thanks in advance and best regards Christoph
RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
Hi, I've got another contribution for cleaning up the jdk.jdi classes. This time it's about package com.sun.tools.jdi. Bug: https://bugs.openjdk.java.net/browse/JDK-8183012 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183012.0/ What I've done: I've largely cleaned up the import statements (removing obsolete imports, expanding "*" wildcards, sorting). Furthermore I did quite a few whitespace cleanups to make the code look nicer. And I removed the types in constructors of typed classes. The more interesting stuff which should probably closely be reviewed is the following: a) Remove unnecessary casts in BooleanValueImpl.java, ByteValueImpl.java, CharValueImpl.java, FloatValueImpl.java, LongValueImpl.java, PacketStream.java, ShortValueImpl.java b) Remove redundant super interfaces in the class declarations of ClassLoaderReferenceImpl.java, ConnectorImpl.java, RawCommandLineLauncher.java, SunCommandLineLauncher.java, ThreadGroupReferenceImpl.java, ThreadReferenceImpl.java c) ObjectReferenceImpl.java: Remove some code in void validateClassMethodInvocation(Method method, int options). Some very old comment suggests that the code was still there because nobody divined what it was useful for. But I couldn't find anything that seems to be really useful here, except if the caller maybe wants to get some exception if such occurred in the course of resolving the class or something like that. d) SocketTransportService.java: pull out SocketConnection to an own file SocketConnection.java, pull class SocketTransportServiceCapabilities into anonymous class within SocketTransportService.capabilities() e) RawCommandLineLauncher.java, SunCommandLineLauncher.java: Modifications to constructing the TransportService object in its constructors Module jdk.jdi still builds without warnings after my change and I'm currently running the jdi jtreg suite. Thanks in advance and best regards Christoph