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