Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Sun, 18 Jun 2023 20:26:04 GMT, Stephen Colebourne
wrote:
>> Roger Riggs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Use {@code xxx} to highlight the comparison against the arg.
>>Update copyrights.
>> - Merge branch 'master' into 8310033-time-compareto
>> - Clarify for Duration, AbstractChronology, and Chronology
>> - Correct javadoc of compareInstant
>> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>>Refine timeline order to mention before and after
>>Add javadoc @see tags to isBefore and isAfter methods
>
> As things stand, this PR makes things worse not better I'm afraid.
> @jodastephen Please review updated descriptions. Thanks
I'm not @jodastephen, obviously, but we both suggested doing something with
punctuation/grammar as this reads funny/peculiar:
@return the comparator value is less than zero
Maybe our suggestions got lost in this huge discussion, or you've decided to
address them later.
-
PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1624431201
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Tue, 20 Jun 2023 18:23:09 GMT, Roger Riggs wrote: > > Where did you get this idea? A "positive" means a number that is _greater_ > > than zero. > > One of my colleagues with a strong math background has corrected many of my > API javadoc comments seeking to avoid any ambiguity. See reference below > about positive integers. > > [home](https://www.math.net/) / [primary > math](https://www.math.net/primary-math) / > [number](https://www.math.net/number) / positive numbers Positive numbers > > A positive number is any number that is greater than 0. **Unlike positive > integers**, which include 0 and the [natural > numbers](https://www.math.net/natural-numbers), positive numbers include > [fractions](https://www.math.net/fraction), > [decimals](https://www.math.net/decimal), and other types of > [numerals](https://www.math.net/numerals). Late to the party. FWIW, the only way I can think of 0 as of a positive integer is when it is represented using fixed-width two's complement (e.g. Java's byte, short, int, and long). In that case, the sign bit for 0 is cleared, just like that of a positive integer and unlike that of a negative integer, assuming the same representation. For example, the following expression will tell you if an `int i` is non-negative, i.e. zero or positive: i >>> 31 == 0 - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1624425467
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Sun, 18 Jun 2023 20:26:04 GMT, Stephen Colebourne
wrote:
>> Roger Riggs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Use {@code xxx} to highlight the comparison against the arg.
>>Update copyrights.
>> - Merge branch 'master' into 8310033-time-compareto
>> - Clarify for Duration, AbstractChronology, and Chronology
>> - Correct javadoc of compareInstant
>> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>>Refine timeline order to mention before and after
>>Add javadoc @see tags to isBefore and isAfter methods
>
> As things stand, this PR makes things worse not better I'm afraid.
@jodastephen Please review updated descriptions. Thanks
-
PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1624350524
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Tue, 20 Jun 2023 20:54:43 GMT, Jens Lidestrom wrote: > > A positive number is any number that is greater than 0. **Unlike positive > > integers**, which include 0 > > math.net seems pretty alone in this. I find the notion bizarre. There seems > to be an overwhelming majority of sources that consider 0 to be neither > negative nor positive. Yes, all the math conventions I'm aware of is that integer zero is neither positive nor negative. For example, the signum method returns 0 for 0, -1 for negative values, and 1 for positive values. - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1609877890
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Sun, 18 Jun 2023 20:24:07 GMT, Stephen Colebourne
wrote:
>> Roger Riggs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Use {@code xxx} to highlight the comparison against the arg.
>>Update copyrights.
>> - Merge branch 'master' into 8310033-time-compareto
>> - Clarify for Duration, AbstractChronology, and Chronology
>> - Correct javadoc of compareInstant
>> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>>Refine timeline order to mention before and after
>>Add javadoc @see tags to isBefore and isAfter methods
>
> src/java.base/share/classes/java/time/MonthDay.java line 678:
>
>> 676: *
>> 677: * @param other the other month-day to compare to, not null
>> 678: * @return the comparator value is less than zero if the {@code
>> other} is before,
>
> Using before/after here could be confusing, as January could be considered to
> be before or after July (since the year is not defined).
The `isBefore` and `isAfter` methods use that terminology and Month is an enum
with defined order January thru December and use the `compareTo` method to
compute before/after.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1237231956
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Tue, 20 Jun 2023 18:23:09 GMT, Roger Riggs wrote: > A positive number is any number that is greater than 0. **Unlike positive > integers**, which include 0 math.net seems pretty alone in this. I find the notion bizarre. There seems to be an overwhelming majority of sources that consider 0 to be neither negative nor positive. - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1599480422
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Sun, 18 Jun 2023 19:33:07 GMT, Jens Lidestrom wrote: > Where did you get this idea? A "positive" means a number that is _greater_ > than zero. One of my colleagues with a strong math background has corrected many of my API javadoc comments seeking to avoid any ambiguity. See reference below about positive integers. [home](https://www.math.net/) / [primary math](https://www.math.net/primary-math) / [number](https://www.math.net/number) / positive numbers Positive numbers A positive number is any number that is greater than 0. **Unlike positive integers**, which include 0 and the [natural numbers](https://www.math.net/natural-numbers), positive numbers include [fractions](https://www.math.net/fraction), [decimals](https://www.math.net/decimal), and other types of [numerals](https://www.math.net/numerals). - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1599291030
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>Refine timeline order to mention before and after
>Add javadoc @see tags to isBefore and isAfter methods
As things stand, this PR makes things worse not better I'm afraid.
src/java.base/share/classes/java/time/Duration.java line 1422:
> 1420: *
> 1421: * @param otherDuration the other duration to compare to, not null
> 1422: * @return the comparator value is less than zero if the {@code
> otherDuration} is before,
There is no concept of before and after when talking about a `Duration`
src/java.base/share/classes/java/time/Instant.java line 1276:
> 1274: * @param otherInstant the other instant to compare to, not null
> 1275: * @return the comparator value is less than zero if the {@code
> otherInstant} is before,
> 1276: * zero if they are equal, greater than zero if the {@code
> otherInstant} is after
Suggestion:
* @return the comparator value - less than zero if {@code otherInstant} is
before this instant,
* zero if they are equal, greater than zero if {@code
otherInstant} is after this instant
Saying `the {@code otherInstant}` doesn't read correctly in text.
`the comparator value` needs punctuation after it in order to make sense
src/java.base/share/classes/java/time/MonthDay.java line 678:
> 676: *
> 677: * @param other the other month-day to compare to, not null
> 678: * @return the comparator value is less than zero if the {@code
> other} is before,
Using before/after here could be confusing, as January could be considered to
be before or after July (since the year is not defined).
src/java.base/share/classes/java/time/OffsetDateTime.java line 1805:
> 1803: *
> 1804: * @param other the other date-time to compare to, not null
> 1805: * @return the comparator value is less than zero if the {@code
> other} is before,
As per `OffsetTime` this definition is wrong. The comparison order is not
before/after.
src/java.base/share/classes/java/time/OffsetTime.java line 1284:
> 1282: *
> 1283: * @param other the other time to compare to, not null
> 1284: * @return the comparator value is less than zero if the {@code
> other} is before,
Using before/after here is very wrong. As described in the text above, two
instances can be at the same point in time, yet still have a defined sort order.
src/java.base/share/classes/java/time/ZoneOffset.java line 717:
> 715: *
> 716: * @param other the other date to compare to, not null
> 717: * @return the comparator value is less than zero if the {@code
> other} is before,
It is tricky to describe an offset as before or after. If you are going to
change this you will need a much better description
src/java.base/share/classes/java/time/chrono/AbstractChronology.java line 659:
> 657: *
> 658: * @param other the other chronology to compare to, not null
> 659: * @return the comparator value is less than zero if the {@code
> other} ID string is before,
A `Chronology` is compared by ID, not before/after
src/java.base/share/classes/java/time/chrono/ChronoZonedDateTime.java line 572:
> 570: *
> 571: * @param other the other date-time to compare to, not null
> 572: * @return the comparator value is less than zero if the {@code
> other} is before,
The comparison is not before/after, as the ID is taken into account
src/java.base/share/classes/java/time/chrono/Chronology.java line 810:
> 808: *
> 809: * @param other the other chronology to compare to, not null
> 810: * @return the comparator value is less than zero if the {@code
> other} ID string is before,
Not before/after
src/java.base/share/classes/java/time/zone/ZoneOffsetTransition.java line 403:
> 401: *
> 402: * @param transition the transition to compare to, not null
> 403: * @return the comparator value is less than zero if the {@code
> transition}
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:54:59 GMT, Pavel Rappo wrote: > While the new wording is clearly an improvement, this reads weird: > > > @return the comparator value is less... > I suggest you use the safe form as the old text, which used a comma: > ``` > @return the comparator value, that is negative... > ``` - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1596245859
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>Refine timeline order to mention before and after
>Add javadoc @see tags to isBefore and isAfter methods
> The term "positive" is ambiguous, zero is considered positive and indicates
> equality.
Where did you get this idea? A "positive" means a number that is _greater_ than
zero.
See both these dictionaries:
https://dictionary.cambridge.org/dictionary/english/positive
https://www.merriam-webster.com/dictionary/positive
-
PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1596244674
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>Refine timeline order to mention before and after
>Add javadoc @see tags to isBefore and isAfter methods
While the new wording is clearly an improvement, this reads weird:
> @return the comparator value is less...
For readability, I'd use _which_ or _that_, depending on your preferred
flavour/flavor of English.
@return the comparator value that is less...
-
PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1595417780
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>Refine timeline order to mention before and after
>Add javadoc @see tags to isBefore and isAfter methods
Using `{@code}` looks good.
-
Marked as reviewed by bpb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14479#pullrequestreview-1484519571
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
> In java.time packages, clarify timeline order javadoc to mention "before" and
> "after" in the value of the `compareTo` method return values.
> Add javadoc @see tags to isBefore and isAfter methods
>
> Replace use of "negative" and positive with "less than zero" and "greater
> than zero" in javadoc @return
> The term "positive" is ambiguous, zero is considered positive and indicates
> equality.
Roger Riggs has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains five additional commits since
the last revision:
- Use {@code xxx} to highlight the comparison against the arg.
Update copyrights.
- Merge branch 'master' into 8310033-time-compareto
- Clarify for Duration, AbstractChronology, and Chronology
- Correct javadoc of compareInstant
- 8310033: Improve Instant.compareTo javadoc to mention before and after
Refine timeline order to mention before and after
Add javadoc @see tags to isBefore and isAfter methods
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14479/files
- new: https://git.openjdk.org/jdk/pull/14479/files/61021307..cd997ede
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14479=01
- incr: https://webrevs.openjdk.org/?repo=jdk=14479=00-01
Stats: 12213 lines in 386 files changed: 3676 ins; 7398 del; 1139 mod
Patch: https://git.openjdk.org/jdk/pull/14479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14479/head:pull/14479
PR: https://git.openjdk.org/jdk/pull/14479
