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

[David Holmes]

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