Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Fri, 12 Feb 2021 22:12:30 GMT, Joe Darcy wrote:
>> 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."
>
> An equivalence relation defines a *set* of equivalence classes which
> partition the objects the relation operates on. For example, the "same signum
> value" equivalence relation on integers has three equivalence classes :
> 1) negative nonzero numbers (corresponding to signum == -1)
> 2) zero (corresponding to signum = 0)
> 3) positive nonzero numbers (corresponding to signum ==1)
OK, got it.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Fri, 12 Feb 2021 22:39:55 GMT, Roger Riggs wrote: >> Added "The string output is not necessary stable over time." The particulars >> of the toString contract of a given class will depend on the class of course. > > Perhaps "necessary" -> "necessarily" Good catch Roger; will change to "The string output is not necessarily stable over time or across JVM invocations." The latter clause will more explicitly handle cases like "myEnumConstant.toString() gave different answers when I ran the program multiple times," a bug that has been submitted as closed as not-a-bug repeated. - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Fri, 12 Feb 2021 22:28:33 GMT, Joe Darcy wrote: >> 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? > > Added "The string output is not necessary stable over time." The particulars > of the toString contract of a given class will depend on the class of course. Perhaps "necessary" -> "necessarily" - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:44:08 GMT, Stuart Marks wrote:
>> 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).
>
> It might be reasonable to add a bit of rationale here and give an example. I
> think the reason that members of the same cohort might not be considered
> `equals()` to one another is that they are not substitutable. For example,
> consider 2.0 and 2.00. They are members of the same cohort, because
>
> new BigDecimal("2.0").compareTo(new BigDecimal("2.00")) == 0
>
> is true. However,
>
> new BigDecimal("2.0").equals(new BigDecimal("2.00"))
>
> is false. They aren't considered `equals()` because they aren't
> substitutable, since using them in an arithmetic expression can give
> different results. For example:
>
> new BigDecimal("2.0").divide(new BigDecimal(3), RoundingMode.HALF_UP)
> ==> 0.7
>
> new BigDecimal("2.00").divide(new BigDecimal(3), RoundingMode.HALF_UP)
> ==> 0.67
>
> I think that's the right rationale, and a reasonable example to illustrate
> it. Edit to taste. I think it would be good to have material like this,
> though, because people's immediate reaction to BD being inconsistent with
> equals is "well that's wrong." Well, no, it's right, and I think this is the
> reason.
Added example using scale and unscaledValue.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:22:18 GMT, Stuart Marks wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix typos in javadoc tags found during review. > > 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? Added "The string output is not necessary stable over time." The particulars of the toString contract of a given class will depend on the class of course. - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:17:48 GMT, Stuart Marks wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix typos in javadoc tags found during review. > > src/java.base/share/classes/java/lang/Object.java line 135: > >> 133: * into equivalence classes; 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" Phrasing updated to: "Members of an equivalence class are substitutable for each other, at least for some purposes." - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:12:06 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> 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."
An equivalence relation defines a *set* of equivalence classes which partition
the objects the relation operates on. For example, the "same signum value"
equivalence relation on integers has three equivalence classes :
1) negative nonzero numbers (corresponding to signum == -1)
2) zero (corresponding to signum = 0)
3) positive nonzero numbers (corresponding to signum ==1)
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:09:16 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> 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.
In BigDecimal, I'll add a sentence:
" The results of methods like {@link scale} and {@link unscaledValue} will
differ for numerically equal values with different representations."
Talking about "precision" and BigDecimal is more appropriate for pre-JSR 13
BigDecimal since values like zero can have the same precision (1 digit) but
many different scales.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:37:34 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> 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.
Okay -- I didn't want to go overboard making all the signum references links to
the method, but I can change the first occurrence in Comparator.equals to a
link.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:19:29 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> 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"
Agree.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:14:08 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> 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.
Sure; most of the terminology in the JDK docs aren't very "math-y".
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:29:11 GMT, Stuart Marks wrote: >> Okay, I see. > > The note itself should be here, but it's demarcated with an `@apiNote` tag. > This introduces a subhead "API Note:" in the rendered javadoc. Thus, it's not > necessary to start the text with "Note:". My thinking here was to include the exact string "Note: this class has a natural ordering that is inconsistent with equals." in the text of BigDecimal since that is the phrase java.lang.Comparable recommends. This should improve grep-ability, etc. for such classes in the JDK. The class-level summary has a semantically equivalent statement using different wording, which left the compareTo method as a place to introduce the exact phrase. I use an @apiNote both because it is an informative statement and to prevent the text from necessarily being applied to BigDecimal subclasses. BigDecimal is not final and people can and sometimes do subclass it and those subclasses, in principle, could have a (different) natural order that was consistent with equals. - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:24:40 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> 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).
It might be reasonable to add a bit of rationale here and give an example. I
think the reason that members of the same cohort might not be considered
`equals()` to one another is that they are not substitutable. For example,
consider 2.0 and 2.00. They are members of the same cohort, because
new BigDecimal("2.0").compareTo(new BigDecimal("2.00")) == 0
is true. However,
new BigDecimal("2.0").equals(new BigDecimal("2.00"))
is false. They aren't considered `equals()` because they aren't substitutable,
since using them in an arithmetic expression can give different results. For
example:
new BigDecimal("2.0").divide(new BigDecimal(3), RoundingMode.HALF_UP)
==> 0.7
new BigDecimal("2.00").divide(new BigDecimal(3), RoundingMode.HALF_UP)
==> 0.67
I think that's the right rationale, and a reasonable example to illustrate it.
Edit to taste. I think it would be good to have material like this, though,
because people's immediate reaction to BD being inconsistent with equals is
"well that's wrong." Well, no, it's right, and I think this is the reason.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Wed, 10 Feb 2021 01:49:55 GMT, Joe Darcy 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 equivalence classes; 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 obje
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Wed, 10 Feb 2021 02:55:14 GMT, Brian Burkhalter wrote: >> This is the exact text recommended in java.lang.Comparable when a type's >> natural ordering is inconsistent with equals. The statement to that effect >> at the top of BigDecimal didn't use that exact wording > > Okay, I see. The note itself should be here, but it's demarcated with an `@apiNote` tag. This introduces a subhead "API Note:" in the rendered javadoc. Thus, it's not necessary to start the text with "Note:". - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Wed, 10 Feb 2021 01:55:24 GMT, Joe Darcy wrote: >> src/java.base/share/classes/java/math/BigDecimal.java line 3062: >> >>> 3060: >>> 3061: * @apiNote >>> 3062: * Note: this class has a natural ordering that is inconsistent >>> with equals. >> >> Is `Note: ` really needed? > > This is the exact text recommended in java.lang.Comparable when a type's > natural ordering is inconsistent with equals. The statement to that effect at > the top of BigDecimal didn't use that exact wording Okay, I see. - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Wed, 10 Feb 2021 01:49:55 GMT, Joe Darcy 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. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Tue, 9 Feb 2021 19:41:25 GMT, Brian Burkhalter wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix typos in javadoc tags found during review. > > src/java.base/share/classes/java/lang/Object.java line 148: > >> 146: * relation, each equivalence class only has a single element. >> 147: * >> 148: * @apiNoted > > `s/apiNoted/apiNote/` Fixed. I did run a successful docs builds before sending out the change for review and I'm surprised the unrecognized tags didn't trigger a docs build failure. thanks. > src/java.base/share/classes/java/math/BigDecimal.java line 3062: > >> 3060: >> 3061: * @apiNote >> 3062: * Note: this class has a natural ordering that is inconsistent >> with equals. > > Is `Note: ` really needed? This is the exact text recommended in java.lang.Comparable when a type's natural ordering is inconsistent with equals. The statement to that effect at the top of BigDecimal didn't use that exact wording - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2471/files - new: https://git.openjdk.java.net/jdk/pull/2471/files/f0d4d2b7..2a8dd8ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2471&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2471&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2471/head:pull/2471 PR: https://git.openjdk.java.net/jdk/pull/2471
