RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[Chris Plummer]

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