Request for review (#4) 7174978: NPG: Fix bactrace builder for class redefinition

[Coleen Phillimore]

On 01/17/2013 06:27 AM, David Holmes wrote:
> On 17/01/2013 6:48 AM, Coleen Phillimore wrote:
>> On 1/16/2013 2:19 AM, David Holmes wrote:
>>>>> src/share/vm/oops/constantPool.hpp
>>>>>
>>>>> ! void add_version(int version) {
>>>>> ! if (version == -1) {
>>>>> ! _saved._version = version; // keep overflow
>>>>> ! } else {
>>>>> ! int new_version = version++;
>>>>> ! _saved._version = version == INT_MAX ? -1 : version;
>>>>> ! }
>>>>> ! }
>>>>>
>>>>> new_version is unused here but I think you meant to do
>>>>>
>>>>> _saved._version = new_version == INT_MAX ? -1 : version;
>>>>>
>>>>
>>>> This was a pre-code-review cleanup... I didn't mean to assign 
>>>> version++
>>>> into new_version. The second line stays the same. I retested this fix.
>>>
>>> I'm unclear what the final form of this code now looks like.
>>
>> void add_version(int version) {
>> if (version == -1) {
>> _saved._version = version; // keep overflow
>> } else {
>> version++;
>> _saved._version = (version == INT_MAX) ? -1 : version;
>> }
>> }
>
> But now you "overflow" too early - version==INT_MAX is not an 
> overflow. If you check the preincrement value against INT_MAX then you 
> have an overflow on the increment - hence I assume you intended
>
>  int new_version = version++;
>  _saved._version = new_version == INT_MAX ? -1 : version;

Actually, I want to save version++ in saved_version.  I want to add one 
to the version passed in.    The version of the constant pool passed in 
is from the old version of the constant pool.

Should I rename this function increment_version(version)?  That would 
probably be more clear. Maybe I should check for overflow by letting it 
go negative, like we do in the symbol.hpp _refcount.

void increment_and_save_version(int version) {
   _saved._version = (version >=0) ? version++ : version;  // keep 
overflowed value
}

Thanks,
Coleen
>
> Thanks,
> David