<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#ffffff">
Thanks for the review in any case! :)<br>
<br>
Jiangli<br>
<br>
On 01/25/2012 10:34 AM, Vitaly Davidovich wrote:
<blockquote
cite="mid:CAHjP37GgHJ52O8dtgsa2jGwummfO_4Kv3p-xyB8RUX52UcNPew@mail.gmail.com"
type="cite">
<p>looks good although I'm not a reviewer :)</p>
<p>Vitaly</p>
<p>Sent from my phone</p>
<div class="gmail_quote">On Jan 25, 2012 1:33 PM, "Jiangli Zhou"
<<a moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com">jiangli.zhou@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
Hi Vitaly and David,<br>
<br>
I've updated the webrev to do a == check via the cast.<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.02/"
target="_blank">http://cr.openjdk.java.net/~jiangli/7132690/webrev.02/</a><br>
<br>
Thanks,<br>
<br>
Jiangli<br>
<br>
On 01/24/2012 11:44 PM, David Holmes wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
On 25/01/2012 2:41 PM, Vitaly Davidovich wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
Hi Jiangli,<br>
<br>
I think doing the comparison via the cast and == check to
make sure it<br>
"roundtrips" is better than explicit range check because
this way you<br>
don't have to change the assert if you change the width of
the type<br>
(e.g. using u2 instead of u1 would require updating the
range). Plus as<br>
Dean mentioned, it looks like the cast version is already
used in the<br>
codebase.<br>
</blockquote>
<br>
Plus the range check assumes that the enum will never have
negative values (not likely here but why preclude it).<br>
<br>
David<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
Cheers<br>
<br>
Sent from my phone<br>
<br>
On Jan 24, 2012 10:19 PM, "Jiangli Zhou" <<a
moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com" target="_blank">jiangli.zhou@oracle.com</a><br>
<mailto:<a moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com" target="_blank">jiangli.zhou@oracle.com</a>>>
wrote:<br>
<br>
__<br>
Hi Vitaly,<br>
<br>
Thanks for catching it! A range check seems to be more
explicit in<br>
this case:<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.02/"
target="_blank">http://cr.openjdk.java.net/~jiangli/7132690/webrev.02/</a><br>
<br>
Thanks,<br>
<br>
Jiangli<br>
<br>
On 01/24/2012 04:17 PM, Vitaly Davidovich wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<br>
I think you'll be fully covered (including negative
values) by<br>
doing the following instead of hard coding max value
in the assert:<br>
<br>
assert(t == (u1)t, "doesn't fit")<br>
<br>
Sent from my phone<br>
<br>
On Jan 24, 2012 6:33 PM, "Jiangli Zhou" <<a
moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com" target="_blank">jiangli.zhou@oracle.com</a><br>
<mailto:<a moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com" target="_blank">jiangli.zhou@oracle.com</a>>>
wrote:<br>
<br>
The updated webrev is :<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.01/"
target="_blank">http://cr.openjdk.java.net/~jiangli/7132690/webrev.01/</a><br>
<<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.01/"
target="_blank">http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.01/</a>><br>
<br>
Thanks,<br>
Jiangli<br>
<br>
On 01/24/2012 02:54 PM, Jiangli Zhou wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt
0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;"> Hi Vitaly,<br>
<br>
An assert in the setter probably is a good
idea. I should<br>
have added it when making the change. Thanks
for the comments!<br>
<br>
Thanks,<br>
Jiangli<br>
<br>
On 01/24/2012 02:13 PM, Vitaly Davidovich
wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt
0pt 0.8ex; border-left: 1px solid rgb(204, 204,
204); padding-left: 1ex;">
<br>
Hi Jiangli,<br>
<br>
This is probably overly paranoid so feel free
to ignore, but<br>
should the setter in InstanceKlass assert
that the passed in<br>
ReferenceType fits into a u1 instead of
silently narrowing<br>
it? Or change the setter to take a u1 and
make caller do the<br>
cast? This would prevent someone defining
another member of<br>
the enum with an explicit value that doesn't
fit into u1.<br>
Like I said, paranoia ... :)<br>
<br>
Thanks<br>
<br>
Sent from my phone<br>
<br>
On Jan 24, 2012 3:06 PM, "Jiangli Zhou"<br>
<<a moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com"
target="_blank">jiangli.zhou@oracle.com</a>
<mailto:<a moz-do-not-send="true"
href="mailto:jiangli.zhou@oracle.com"
target="_blank">jiangli.zhou@oracle.com</a>>><br>
wrote:<br>
<br>
Hi,<br>
<br>
Please review the change for 7132690:<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.00/"
target="_blank">http://cr.openjdk.java.net/~jiangli/7132690/webrev.00/</a><br>
<<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.00/"
target="_blank">http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.00/</a>><br>
<br>
It changes InstanceKlass::_reference_type
from<br>
ReferenceType to u1. The ReferenceType is
defined as an<br>
enum with 6 values<br>
(src/share/vm/utilities/globalDefinitions.hpp).
The<br>
change saves 4-byte for each class.<br>
<br>
Tested with runThese and ute
vm.quick.testlist. Ran jprt.<br>
<br>
Thanks,<br>
<br>
Jiangli<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>