[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 10 (L): 8183012: Code cleanup in com.sun.tools.jdi
From: Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date: 2017-06-28 14:56:01
Message-ID: CAA-vtUz1ruSkarb55W0nX9G-D43J8+E_uSR39N3DC3LebFngFA () mail ! gmail ! com
[Download RAW message or body]
Hi Christoph,
On Wed, Jun 28, 2017 at 4:47 PM, Langer, Christoph <christoph.langer@sap.com
> 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 = 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?
>
> 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
[Attachment #3 (text/html)]
<div dir="ltr">Hi Christoph,<div><br></div><div><br></div><div \
class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 28, 2017 at 4:47 PM, \
Langer, Christoph <span dir="ltr"><<a href="mailto:christoph.langer@sap.com" \
target="_blank">christoph.langer@sap.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Hi Thomas,<br> <br>
thanks for your review!<br>
<span class=""><br>
> com/sun/tools/jdi/<wbr>InvokableTypeImpl.java:<br>
><br>
> import com.sun.jdi.<wbr>VMCannotBeModifiedException; ?Why do we need to<br>
> import this type, we do not seem to use it?<br>
<br>
</span>This is needed for the documentation, see line 97, the throws \
documentation.<br> <span class=""><br>
> ObjectReferenceImpl:<br>
><br>
> the removed code: Looks like part of the checks are missing since a long \
time.<br> > The remains could be understood as a check that the class for the \
method to<br> > be invoked exists in the debuggee. But then, there are no \
guarantees<br> > anyway that inbetween firing the invoke command to the debuggee \
and the<br> > debuggee processing the invoke command that class may not be \
unloaded,<br> > so we may just rely on the jdwp invoke command failing for that \
case. So, ok<br> > to remove it IMHO.<br>
<br>
</span>I thought so, too.<br>
<span class=""><br>
> SDE.java:<br>
><br>
> @SuppressWarnings("unused") ? which member is unused?<br>
<br>
</span>It's about member "njplsEnd". Still it looks cleaner not to \
remove it but to suppress the warning.<br> <span class=""><br>
> "SocketTransportService.java: pull out SocketConnection to an own file<br>
> SocketConnection.java" - why?<br>
<br>
</span>In the codebase that I was merging the package with, the class \
SocketConnection needed to be declared public for some reason.<br> 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<br> 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. :)<br> It will ease future merging.<br>
<span class=""><br>
> com/sun/tools/jdi/VMModifiers.<wbr>java:<br>
><br>
> Not touched by your change, but weird: In VMModifiers, some of the<br>
> constants share numerical values:<br>
><br>
> 28 public interface VMModifiers {<br>
> ...<br>
> 35 int VOLATILE = 0x00000040; /* can cache in registers */<br>
> 36 int BRIDGE = 0x00000040; /* Bridge method generated by \
compiler<br> > */<br>
><br>
> 37 int TRANSIENT = 0x00000080; /* not persistant */<br>
> 38 int VARARGS = 0x00000080; /* Method accepts var. args*/<br>
> ...<br>
><br>
> So, VOLATILE == BRIDGE and TRANSIENT == VARARGS? This seems wrong, no?<br>
<br>
</span>This really looks weird, I agree. But it's out of scope for me to dig \
further... :)<br> <br>
As I addressed all the points you mentioned, I will consider the change reviewed by \
you.<br> <br>
Best regards<br>
<span class="HOEnZb"><font color="#888888">Christoph<br>
<br></font></span></blockquote><div><br></div><div>Thanks for the answers, and I am \
fine with the change, no need for a new webrev.</div><div><br></div><div>Kind \
Regards, Thomas</div><div> </div></div><br></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic