[REFRESH] JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger

[Stuart Marks]

Hi Brian,

Just a couple small items.

At line 4203, the type of magnitude should be byte[] instead of int[]. Whoops, I 
could have sworn I wrote that in my previous review, but it must have gotten 
dropped while I was editing. Sorry about that.

For the four compatibility fields in the serial form, the comment is

     appears in the serialized for backward compatibility

Something is missing here. Should it say "appears in the serialized form for 
backward compatibility" ?

The comment block at lines 4300-4306 is good. I might also add a note to say 
these values are compatible with older implementations.

>> There are some things in the serialization doc that ought to be brought up to date, though. Note that the docs for serialPersistentFields, readObject, and writeObject appear in the javadoc output, in the "Serialized Form" page, even though these members are private!
>
> Isn't this controlled by options passed to the javadoc tool as opposed to settings in the source code?

No, serialization is "special" in that all information about the serialized 
form, including the docs these special private methods and fields, do appear in 
the Serialized Form output, regardless of the javadoc tool arguments.

> I think I'll need another "thumbs up" as this has changed since Paul's approval was posted.

Paul is not available this week. If you want to make these corrections and then 
just push the changeset, it's fine by me; I think it's had enough review.

s'marks


On 3/3/14 11:37 AM, Brian Burkhalter wrote:
> Hi Stuart,
>
> Thanks for the detailed review!
>
> Please see the refreshed webrev
> http://cr.openjdk.java.net/~bpb/8035279/webrev.02/ and my comments inline, below.
>
> I think I'll need another "thumbs up" as this has changed since Paul's approval
> was posted.
>
> On Feb 28, 2014, at 5:35 PM, Stuart Marks wrote:
>
>>> Thanks, Paul. I refreshed the webrev
>>> http://cr.openjdk.java.net/~bpb/8035279/webrev.01/ with the agreed upon version.
>>
>> This is pretty good. After this long, strange trip through the JMM, restoring
>> the sentinel values to zeroes and renaming the fields to be explicit about how
>> they represent the actual values seems to be the best approach. Paul's
>> suggestion about using the term "stable value" in comments is good too.
>>
>> I took a look at the serialization stuff. The actual serialized form hasn't
>> changed, so there should be no compatibility here with previous versions.
>>
>> There are some things in the serialization doc that ought to be brought up to
>> date, though. Note that the docs for serialPersistentFields, readObject, and
>> writeObject appear in the javadoc output, in the "Serialized Form" page, even
>> though these members are private!
>
> Isn't this controlled by options passed to the javadoc tool as opposed to
> settings in the source code?
>
>> Per another of Paul's comments, the @serial tag should be removed from
>> bitCountPlusOne, bitLengthPlusOne, and lowestSetBitPlusTwo, since these fields
>> do not appear in the serialized representation.
>
> Corrected.
>
>> The fields bitCount, bitLength, and lowestSetBit appear in the serialized form
>> only for backward compatibility and are otherwise ignored, so their
>> @serialField entries should just say that instead of describing how they were
>> formerly used. Also, firstNonzeroByteNum is missing a @serialField entry, and
>> it should have the same description as the others.
>
> Corrected.
>
>> Typo at 4236-4237, it says "be\ndefault" instead of "by\ndefault".
>
> Corrected.
>
>> The comment at lines 4242-4246 should simply be removed. The first and third
>> sentences are redundant with other docs. The second sentence, "The magnitude
>> field is used as a temporary store for the byte array that is deserialized" is
>> incorrect, as there is no longer a 'magnitude' field; a local is used instead.
>
> Corrected.
>
>> The @serialData tag at line 4316 for writeObject is misused; this is really
>> intended for *extra* serial data written by writeObject after the
>> writeFields() or defaultWriteObject() call, which doesn't occur here. It might
>> be worth being explicit in writeObject's doc comment about writing -1's and
>> -2's as the values for bitCount, bitLength, lowestSetBit, and
>> firstNonzeroByteNum for compatibility with older implementations, even though
>> current implementations will ignore these values.
>
> Corrected.
>
> On Feb 28, 2014, at 5:54 PM, Stuart Marks wrote:
>
>> Woops, I forgot a couple points.
>>
>> The @serial for the 'signum' field at line 130 should be removed. Note that
>> even though this has the same name as the field that appears in the serialized
>> form, the text from the @serialField tag for 'signum' that's part of the
>> serialPersistentFields doc comment is the one that actually appears on the
>> "Serialized Form" page.
>
> Corrected.
>
>> It might be worthwhile copying the more verbose description from lines 125-128
>> to the @serialField tag text (lines 4206-4207) since this describes the
>> requirements on the serialized form more precisely. I hate redundancy in
>> documentation, though.
>
> I opted for not copying the verbiage in the interested of reduced redundancy.
>
>> Note that the field names in the ObjectStreamField entries of the
>> serialPersistentFields array don't necessarily match the actual fields in the
>> class. In this case 'signum' matches but the others do not. That's ok. The
>> entries in this array describe the field names that are used in the serialized
>> output, which is essential for remaining compatible with older versions of
>> BigInteger.
>
> Understood.
>
>> I checked an old version of BigInteger from 1998 and the field names used here
>> match the actual, serialized fields from that old version.
>
> I verified compatibility with the Java 7 version but no older so this is good
> information.
>
> Thanks,
>
> Brian