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 = 0x00000040; /* can cache in registers */
36 int BRIDGE = 0x00000040; /* Bridge method generated by
compiler */
37 int TRANSIENT = 0x00000080; /* not persistant */
38 int VARARGS = 0x00000080; /* 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, Christoph <christoph.lan...@sap.com
> 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
>
>
>
