Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Thank you very much, Chris, for taking a look.
Yes, I have the arch specific check since VM_Version::CPU_SHA is cpu
specific. I chose one platform independent long constant --
"markOopDesc::hash_mask_in_place". This is the only platform independent
long Constant with which the truncation issue can be observed. I added a
platform dependent long constant also to get some extra testing done.
I will add the comments for the new code.
Thank you,
Jini.
On 5/12/2018 6:30 AM, Chris Plummer wrote:
Hi Jini,
Why do you have the following:
98 if (arch.equals("amd64") || arch.equals("i386") ||
arch.equals("x86")) {
Is it because VM_Version::CPU_SHA is only expected on these platforms?
If so, is there a reason you didn't choose a long constant that exists
on all platforms?
And just one minor nit. I think the new code warrants some comments
indicating what it is trying to test for.
thanks,
Chris
On 5/11/18 5:11 AM, Jini George wrote:
Thank you, David. Could I get another pair of eyes to take a look at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
Thanks,
Jini.
On 5/11/2018 11:58 AM, David Holmes wrote:
Forgot to add - no need to see an updated webrev.
Thanks,
David
On 11/05/2018 4:22 PM, David Holmes wrote:
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the clarifications
offline! I have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the
expected values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to
int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen
on 32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we
need to look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n
or \r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters.
Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you
can just:
throw new Exception("Reading XXX: got value " + value + " but
expected " + expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Thank you very much, Serguei. Will add it.
- Jini.
On 5/12/2018 6:29 AM, serguei.spit...@oracle.com wrote:
Hi Jini,
It looks good to me.
A minor comment on the ClhsdbLongConstant.java.frames.html:
Could you add a comment with an example of the expected longConstantOutput?
I do not need another webrev if you add it.
Thanks,
Serguei
On 5/11/18 05:11, Jini George wrote:
Thank you, David. Could I get another pair of eyes to take a look at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
Thanks,
Jini.
On 5/11/2018 11:58 AM, David Holmes wrote:
Forgot to add - no need to see an updated webrev.
Thanks,
David
On 11/05/2018 4:22 PM, David Holmes wrote:
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the clarifications
offline! I have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the
expected values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to
int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen
on 32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we
need to look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n
or \r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters.
Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you
can just:
throw new Exception("Reading XXX: got value " + value + " but
expected " + expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini,
Why do you have the following:
98 if (arch.equals("amd64") || arch.equals("i386") ||
arch.equals("x86")) {
Is it because VM_Version::CPU_SHA is only expected on these platforms?
If so, is there a reason you didn't choose a long constant that exists
on all platforms?
And just one minor nit. I think the new code warrants some comments
indicating what it is trying to test for.
thanks,
Chris
On 5/11/18 5:11 AM, Jini George wrote:
Thank you, David. Could I get another pair of eyes to take a look at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
Thanks,
Jini.
On 5/11/2018 11:58 AM, David Holmes wrote:
Forgot to add - no need to see an updated webrev.
Thanks,
David
On 11/05/2018 4:22 PM, David Holmes wrote:
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the clarifications
offline! I have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the
expected values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to
int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen
on 32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we
need to look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n
or \r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters.
Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you
can just:
throw new Exception("Reading XXX: got value " + value + " but
expected " + expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini,
It looks good to me.
A minor comment on the ClhsdbLongConstant.java.frames.html:
Could you add a comment with an example of the expected longConstantOutput?
I do not need another webrev if you add it.
Thanks,
Serguei
On 5/11/18 05:11, Jini George wrote:
Thank
you, David. Could I get another pair of eyes to take a look at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
Thanks,
Jini.
On 5/11/2018 11:58 AM, David Holmes wrote:
Forgot to add - no need to see an updated
webrev.
Thanks,
David
On 11/05/2018 4:22 PM, David Holmes wrote:
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the
clarifications offline! I have addressed your comments and
have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the
expected values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates
values to int)
Webrev:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would
happen on 32-bit as well.
- CheckForTruncation seems rather complicated, not sure
why we need to look for two different things with one
being amd64 specific??
- We've had discussions in the past about splitting
strings on \n or \r\n depending on whether it's Windows or
not. Better to use String.split("\R") regex function.
- Exception message strings should not contain newline
characters. Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String
message" you can just:
throw new Exception("Reading XXX: got value " + value + "
but expected " + expected);
Thanks,
David
-
Testing: The SA tests passed on
Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Thank you, David. Could I get another pair of eyes to take a look at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html
Thanks,
Jini.
On 5/11/2018 11:58 AM, David Holmes wrote:
Forgot to add - no need to see an updated webrev.
Thanks,
David
On 11/05/2018 4:22 PM, David Holmes wrote:
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the clarifications
offline! I have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the expected
values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen on
32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we need
to look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n or
\r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters.
Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you
can just:
throw new Exception("Reading XXX: got value " + value + " but
expected " + expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Forgot to add - no need to see an updated webrev.
Thanks,
David
On 11/05/2018 4:22 PM, David Holmes wrote:
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the clarifications offline!
I have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the expected
values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen on
32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we need
to look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n or
\r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters.
Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you
can just:
throw new Exception("Reading XXX: got value " + value + " but
expected " + expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini,
On 11/05/2018 5:34 AM, Jini George wrote:
Hi David,
Thank you very much for the review and for the clarifications offline! I
have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Looks fine.
A couple of minor comments on the test:
You don't need:
79 LingeredApp.stopApp(theApp);
as it is done in the finally block.
Can you add comments in checkForTruncation stating where the expected
values were obtained from.
Thanks,
David
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen on
32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we need to
look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n or
\r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters.
Also you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you can
just:
throw new Exception("Reading XXX: got value " + value + " but expected
" + expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi David,
Thank you very much for the review and for the clarifications offline! I
have addressed your comments and have a new webrev at:
http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html
Thanks,
Jini.
On 5/4/2018 12:23 PM, David Holmes wrote:
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen on
32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we need to
look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n or
\r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters. Also
you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you can
just:
throw new Exception("Reading XXX: got value " + value + " but expected "
+ expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini,
On 4/05/2018 2:17 AM, Jini George wrote:
Hello!
Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8195613
([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)
Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/
Actual fix seems fine.
I have a few comments on the test ...
- Why are you only testing on 64-bit? The truncation would happen on
32-bit as well.
- CheckForTruncation seems rather complicated, not sure why we need to
look for two different things with one being amd64 specific??
- We've had discussions in the past about splitting strings on \n or
\r\n depending on whether it's Windows or not. Better to use
String.split("\R") regex function.
- Exception message strings should not contain newline characters. Also
you can simplify them:
Reading XXX: got value NNN but expected MMM
And you don't need to define a local variable "String message" you can just:
throw new Exception("Reading XXX: got value " + value + " but expected "
+ expected);
Thanks,
David
-
Testing: The SA tests passed on Mach5.
Thanks,
Jini.
RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Testing: The SA tests passed on Mach5. Thanks, Jini.
