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
