Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi

[Thomas Stüfe]

Hi Christoph,



On Wed, Jun 28, 2017 at 4:47 PM, Langer, Christoph  wrote:

> 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

[Langer, Christoph]

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

[Thomas Stüfe]

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, 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

[serguei.spit...@oracle.com]

  
  
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

[Langer, Christoph]

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