On Wed, 10 Feb 2021 01:49:55 GMT, Joe Darcy <da...@openjdk.org> wrote:
>> A follow-up of sorts to JDK-8257086, this change aims to improve the
>> discussion of the relationship between Object.equals and compareTo and
>> compare methods. The not-consistent-with-equals natural ordering of
>> BigDecimal get more explication too. While updating Object, I added some
>> uses of apiNote and implSpec, as appropriate. While a signum method did not
>> exist when Comparable was added, it has existed for many years so I removed
>> obsolete text that defined a "sgn" function to compute signum.
>>
>> Once the changes are agreed to, I'll reflow paragraphs and update the
>> copyright years.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix typos in javadoc tags found during review.
Overall very nice. Mostly my comments are wordsmithing. Two issues are worth
considering; see detailed comments inline.
1) Do we want a general statement on the stability of the value returned by
toString()?
2) Add rationale and example for why BigDecimal's natural ordering is
inconsistent with equals.
src/java.base/share/classes/java/lang/Comparable.java line 68:
> 66: * orderings that are consistent with equals. One exception is
> 67: * {@link java.math.BigDecimal}, whose {@linkplain
> java.math.BigDecimal#compareTo natural ordering} equates
> 68: * {@code BigDecimal} objects with equal numerical values and different
> representations
Nothing here talks about the representation of BigDecimal. I think it would be
preferable to say that in BigDecimal, the `equals()` method considers 4.0 and
4.00 unequal because they have different precisions; however, `compareTo()`
considers them equal because it considers only their numerical values.
src/java.base/share/classes/java/lang/Comparable.java line 90:
> 88: * of the {@code equals} method and the equivalence classes defined by
> 89: * the quotient of the {@code compareTo} method are the same.
> 90: *
I think in both cases it should be "the equivalence class defined by...." That
is, "equivalence class" should be singular. But there are two of them, so the
sentence still properly concludes "... are the same."
src/java.base/share/classes/java/lang/Comparable.java line 110:
> 108: * {@link Integer#signum signum}{@code (x.compareTo(y)) ==
> -signum(y.compareTo(x))}
> 109: * for all {@code x} and {@code y}. (This
> 110: * implies that {@code x.compareTo(y)} must throw an exception iff
I'd suggest replacing "iff" with "if and only if" because it looks like a typo,
and writing out the long form emphasizes the bidirectional nature of the
implication.
src/java.base/share/classes/java/lang/Object.java line 135:
> 133: * into <i>equivalence classes</i>; all the members of an
> 134: * equivalence class are equal to each other. The equal objects
> 135: * are substitutable for each other, at least for some purposes.
Since the preceding sentence mentions the members of an equivalence class, it
might read better to say "Members are substitutable..." or possibly "Members of
an equivalence class are substitutable...."
src/java.base/share/classes/java/lang/Object.java line 149:
> 147: *
> 148: * @apiNote
> 149: * Note that it is generally necessary to override the {@link
> hashCode hashCode}
The `@apiNote` introduces the paragraph with a subhead "API Note:" so it's a
bit redundant to say "Note that...." Maybe just start with "It is generally
necessary...."
src/java.base/share/classes/java/lang/Object.java line 236:
> 234: * be a concise but informative representation that is easy for a
> 235: * person to read.
> 236: * It is recommended that all subclasses override this method.
Do we want to have a general note here or somewhere that the exact format of
the result of `toString()` is generally not stable, and that it is subject to
change without notice?
src/java.base/share/classes/java/math/BigDecimal.java line 97:
> 95: * contrast, the {@link equals equals} method requires both the
> 96: * numerical value and representation to be the same for equality to
> 97: * hold.
Note, discussing "representation" is ok here since the context is discussing
the representation of BigDecimal (in contrast to the text in Comparable).
src/java.base/share/classes/java/util/Comparator.java line 95:
> 93: * equals, the equivalence classes defined by the equivalence relation
> 94: * of the {@code equals} method and the equivalence classes defined by
> 95: * the quotient of the {@code compare} method are the same.
As before, suggest "equivalence class" be singular in both cases.
src/java.base/share/classes/java/util/Comparator.java line 159:
> 157: * and it imposes the same ordering as this comparator. Thus,
> 158: * {@code comp1.equals(comp2)} implies that {@code
> signum(comp1.compare(o1,
> 159: * o2))==signum(comp2.compare(o1, o2))} for every object reference
Maybe make "signum" be a link here, similar to other locations where it's used.
-------------
Marked as reviewed by smarks (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2471
