<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Apr 28, 2010, at 2:23 AM, Christian Thalinger wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Fri, 2010-04-23 at 15:52 -0700, John Rose wrote:<br><blockquote type="cite">On Apr 23, 2010, at 7:06 AM, Christian Thalinger wrote:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">I have to say I was a little confused about the get_index1,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">get_index2, ... naming as it looks like you were running out of method<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">names and just append 1, 2, ... ;-)<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">You are right, it could look that way at first. &nbsp;Can you suggest some<br></blockquote><blockquote type="cite">comments that would short-circuit that impression quickly?<br></blockquote><br>Well, you could rename it to get_index_u{1,2,4} which would correspond<br>to the other Bytes::get* functions.</div></blockquote><div><br></div><div>Thanks, I like this idea. &nbsp;I changed it to be this way.</div><div><br></div><div>One wrinkle: get_offset2 changes to get_offset_s2 (since it's signed).</div><div>I think it is good to advertise the signed-ness of the accessors.</div><br><blockquote type="cite"><div>If you want to keep them maybe add a simple comment like:<br><br>// get_{index,offset,constant}{1,2,4} methods return an<br>// index/offset/constant of the size of the appended number in bytes <br>//(1, 2, or 4 bytes).<br><br>You may rephrase for better English.<br><br>I just see that ciStreams.hpp has comments on most of these methods.<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">The change is very big. &nbsp;I take a look at it again later.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">That will be just fine.<br></blockquote><br>src/share/vm/prims/jvmtiClassFileReconstituter.cpp:<br><br>Copyright year update missing.</div></blockquote><div><br></div><div>Fixed.</div><br><blockquote type="cite"><div>src/share/vm/prims/methodComparator.cpp:<br><br>- &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if (_s_old-&gt;get_index_big() != _s_new-&gt;get_index_big())<br>+ &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if (Bytes::get_Java_u2(_s_old-&gt;bcp() + 1) != Bytes::get_Java_u2(_s_new-&gt;bcp() + 1))<br><br>Why can't you use get_index2() here?</div></blockquote><div><br></div><div>It's a pre-existing hack. &nbsp;I added comments. &nbsp;I thought about splitting up the comparisons, but didn't want to risk a bug.</div><div><br></div><div><font class="Apple-style-span" face="'Lucida Sans Typewriter'" size="3"><span class="Apple-style-span" style="font-size: 12px;">&nbsp;&nbsp; &nbsp;if (! _s_old-&gt;is_wide()) {<br>&nbsp;&nbsp; &nbsp; &nbsp;// We could use get_index_u1 and get_constant_u1, but it's simpler to grab both bytes at once:<br>&nbsp;&nbsp; &nbsp; &nbsp;if (Bytes::get_Java_u2(_s_old-&gt;bcp() + 1) != Bytes::get_Java_u2(_s_new-&gt;bcp() + 1))<br>&nbsp;&nbsp; &nbsp; &nbsp; &nbsp;return false;<br>&nbsp;&nbsp; &nbsp;} else {<br>&nbsp;&nbsp; &nbsp; &nbsp;// We could use get_index_u2 and get_constant_u2, but it's simpler to grab all four bytes at once:<br>&nbsp;&nbsp; &nbsp; &nbsp;if (Bytes::get_Java_u4(_s_old-&gt;bcp() + 1) != Bytes::get_Java_u4(_s_new-&gt;bcp() + 1))<br>&nbsp;&nbsp; &nbsp; &nbsp; &nbsp;return false;<br>&nbsp;&nbsp; &nbsp;}<br></span></font><br></div><blockquote type="cite"><div>Otherwise looks good.</div></blockquote></div><br><div>Thanks!</div><div><br></div><div>-- John</div></body></html>