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.
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.
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. I checked an old version of BigInteger from 1998 and the field names
used here match the actual, serialized fields from that old version.
s'marks
On 2/28/14 5:35 PM, Stuart Marks wrote:
On 2/27/14 12:14 PM, Brian Burkhalter wrote:
http://cr.openjdk.java.net/~bpb/8035279/webrev.01/ with the agreed upon version.
Hi Brian,
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!
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.
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.
Typo at 4236-4237, it says "be\ndefault" instead of "by\ndefault".
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.
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.
Thanks,
s'marks
