Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8310033: Clarify return value of Java Time compareTo methods #14479

Closed
wants to merge 14 commits into from

Conversation

RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Jun 14, 2023

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8310064 to be approved

Issues

  • JDK-8310033: Clarify return value of Java Time compareTo methods (Bug - P4)
  • JDK-8310064: Clarify Java Time documentation of the return value of compareTo methods (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14479/head:pull/14479
$ git checkout pull/14479

Update a local copy of the PR:
$ git checkout pull/14479
$ git pull https://git.openjdk.org/jdk.git pull/14479/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14479

View PR using the GUI difftool:
$ git pr show -t 14479

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14479.diff

Webrev

Link to Webrev Comment

Refine timeline order to mention before and after
Add javadoc @see tags to isBefore and isAfter methods
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 14, 2023

👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jun 14, 2023

@RogerRiggs The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org csr Pull request needs approved CSR before integration labels Jun 14, 2023
@RogerRiggs RogerRiggs changed the title 8310033: Improve Instant.compareTo javadoc to mention before and after 8310033: Clarify return value of Java Time compareTo methods Jun 16, 2023
@RogerRiggs RogerRiggs marked this pull request as ready for review June 16, 2023 19:24
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 16, 2023
@RogerRiggs
Copy link
Contributor Author

/label remove i18n

@openjdk openjdk bot removed the i18n i18n-dev@openjdk.org label Jun 16, 2023
@openjdk
Copy link

openjdk bot commented Jun 16, 2023

@RogerRiggs
The i18n label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented Jun 16, 2023

Copy link
Member

@bplb bplb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@@ -1419,7 +1419,8 @@ public Duration truncatedTo(TemporalUnit unit) {
* It is "consistent with equals", as defined by {@link Comparable}.
*
* @param otherDuration the other duration to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the other duration is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment. I am not sure that "other duration" is quite clear. It seems that javadoc for many compareTo methods have not standardized on wording for describing the argument being checked. Some include "argument" when the documentation. Perhaps change "other duration" -> "duration argument"?

I think LocalDate::compareTo and its use of "other" is perhaps a better example of why we might want to beef the wording up a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument name is otherDuration would putting the argument name in {@code} font put sufficient focus on the argument?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be fine as well as then it is clear we are referring to the argument.

Copy link
Member

@bplb bplb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using {@code} looks good.

@pavelrappo
Copy link
Member

pavelrappo commented Jun 16, 2023

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...

@jensli
Copy link

jensli commented Jun 18, 2023

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

@jensli
Copy link

jensli commented Jun 18, 2023

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...

Copy link
Contributor

@jodastephen jodastephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As things stand, this PR makes things worse not better I'm afraid.

@@ -1419,7 +1419,8 @@ public Duration truncatedTo(TemporalUnit unit) {
* It is "consistent with equals", as defined by {@link Comparable}.
*
* @param otherDuration the other duration to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code otherDuration} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no concept of before and after when talking about a Duration

Comment on lines 1275 to 1276
* @return the comparator value is less than zero if the {@code otherInstant} is before,
* zero if they are equal, greater than zero if the {@code otherInstant} is after
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return the comparator value is less than zero if the {@code otherInstant} is before,
* zero if they are equal, greater than zero if the {@code otherInstant} is after
* @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

@@ -656,7 +656,8 @@ void addFieldValue(Map<TemporalField, Long> fieldValues, ChronoField field, long
* Subclasses must compare any additional state that they store.
*
* @param other the other chronology to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} ID string is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Chronology is compared by ID, not before/after

@@ -714,7 +714,9 @@ public Temporal adjustInto(Temporal temporal) {
* The comparison is "consistent with equals", as defined by {@link Comparable}.
*
* @param other the other date to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -1281,7 +1281,11 @@ public long toEpochSecond(LocalDate date) {
* use {@link ChronoField#NANO_OF_DAY} as a comparator.
*
* @param other the other time to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1801,7 +1802,11 @@ public long toEpochSecond() {
* consistent with {@code equals()}.
*
* @param other the other date-time to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per OffsetTime this definition is wrong. The comparison order is not before/after.

@@ -675,7 +675,11 @@ public LocalDate atYear(int year) {
* It is "consistent with equals", as defined by {@link Comparable}.
*
* @param other the other month-day to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using before/after here could be confusing, as January could be considered to be before or after July (since the year is not defined).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -569,7 +569,11 @@ default long toEpochSecond() {
* This default implementation performs the comparison defined above.
*
* @param other the other date-time to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison is not before/after, as the ID is taken into account

@@ -807,7 +807,8 @@ default boolean isIsoBased() {
* It is "consistent with equals", as defined by {@link Comparable}.
*
* @param other the other chronology to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code other} ID string is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not before/after

@@ -400,7 +400,8 @@ List<ZoneOffset> getValidOffsets() {
* The offsets are ignored, making this order inconsistent with equals.
*
* @param transition the transition to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value is less than zero if the {@code transition} is before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not before/after

@RogerRiggs
Copy link
Contributor Author

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 / primary math / 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, positive numbers include fractions, decimals, and other types of numerals.

@jensli
Copy link

jensli commented Jun 20, 2023

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.

@jddarcy
Copy link
Member

jddarcy commented Jun 27, 2023

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.

@rgiulietti
Copy link
Contributor

Most modern authors in math topics use positive to mean > 0, negative to mean < 0, and 0 is neither.

In some contexts, like in the IEEE 754 standard for floating-point numbers, there are both distinguishable positive and a negative binary zeros. There's no "unsigned" or "agnostic" zero there.

The natural numbers include 0 in foundational topics like set theory (where 0 is defined as the empty set), and exclude it in others.

BTW, math.net contradicts itself later on the same page. In the "Is 0 positive or negative?" section it states "Positive numbers are defined as any number that is greater than zero".

@jddarcy
Copy link
Member

jddarcy commented Jun 27, 2023

Adding a bit more context, floating-point arithmetic is an approximation to real arithmetic. Real arithmetic has a single zero value. The signed zeros of IEEE 754 floating-point are a computational artifact.

More specifically, IEEE 754 floating-point is an approximation to the real numbers extended with infinite values. There are two ways to extend the reals with infinity: affine (signed infinities) and projective (one unsigned infinity). Early drafts of the IEEE 754 standard supported both affine and project infinity, as did the original 8087 floating-point which predated IEEE 754. The final IEEE 754 only supports signed infinities and because there are two infinities, there are two distinguished (but numerically equal) zero values.

The floating-point signum methods in the Java math library return a same-signed zero for a zero input; +1.0 is not returned for +0.0 and -1.0 is not returned for -0.0. While a zero value has a sign bit in its representation, neither zero value is "positive" or "negative" in the mathematical sense of the term.

@RogerRiggs
Copy link
Contributor Author

@jodastephen Please review updated descriptions. Thanks

@pavelrappo
Copy link
Member

pavelrappo commented Jul 6, 2023

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 / primary math / 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, positive numbers include fractions, decimals, and other types of 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

@pavelrappo
Copy link
Member

@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.

value < 0 as this is before that, and > 0 as this is after that.
Thanks to a careful reviewer spotting my reversing of the conditions.
@RogerRiggs
Copy link
Contributor Author

Please re-review; its doc-only so can be still included in JDK 21.

@@ -1807,7 +1807,10 @@ public ZonedDateTime atZone(ZoneId zone) {
* chronology is also considered, see {@link ChronoLocalDateTime#compareTo}.
*
* @param other the other date-time to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value, that is less than zero if this is before {@code other},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text is incorrect. It does not match the description above re chronologies.

@@ -1988,7 +1988,10 @@ public long toEpochSecond(LocalTime time, ZoneOffset offset) {
* chronology is also considered, see {@link java.time.chrono.ChronoLocalDate#compareTo}.
*
* @param other the other date to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value, that is less than zero if this is before {@code other},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text is incorrect. It does not match the description above re chronologies.

@@ -1801,7 +1802,10 @@ public long toEpochSecond() {
* consistent with {@code equals()}.
*
* @param other the other date-time to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value, that is the comparison with the {@code other}'s instant, if they are not equal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text is incorrect. It does not match the description above re offsets. (This would be a suitable description for compareInstant)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1806 describes the comparison of the local time if the instants are equal.
and if equal to the {@code other}'s instant, the comparison of the {@code other}'s local date-time

* @return the comparator value, negative if less, positive if greater
* @return the comparator value, that is the comparison of the UTC equivalent {@code other} instant,
* if they are not equal, and if the UTC equivalent {@code other} instant is equal,
* the comparison of this local date-time with {@code other} local date-time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text is incorrect. It compares the local time, not the local date-time, when the instants are equal

@@ -700,7 +700,10 @@ default long toEpochDay() {
* This default implementation performs the comparison defined above.
*
* @param other the other date to compare to, not null
* @return the comparator value, negative if less, positive if greater
* @return the comparator value, that is less than zero if this is before {@code other},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text is incorrect. It does not match the description above re chronologies.

Copy link
Contributor

@jodastephen jodastephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RogerRiggs
Copy link
Contributor Author

Thank you for your careful review.

Copy link
Member

@pavelrappo pavelrappo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Member

@naotoj naotoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@openjdk
Copy link

openjdk bot commented Jul 27, 2023

@RogerRiggs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8310033: Clarify return value of Java Time compareTo methods

Reviewed-by: bpb, scolebourne, prappo, naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 45 new commits pushed to the master branch:

  • a9d21c6: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
  • 4c2e54f: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails
  • 830413f: 8313087: DerValue::toString should output a hex view of the values in byte array
  • 7412193: 4800398: (ch spec) Clarify Channels.newChannel(InputStream) spec
  • e7726fb: 8313155: Problem list some JUnit-based tests in test/jdk/java/lang/invoke
  • 02a0473: 8312445: Array types in annotation elements show square brackets twice
  • c22cadf: 8312526: Test dk/jfr/event/oldobject/TestHeapDeep.java failed: Could not find ChainNode
  • cc2a75e: 8312619: Strange error message when switching over long
  • 1f81e5b: 8312229: Crash involving yield, switch and anonymous classes
  • e9daf4a: 8312916: Remove remaining usages of -Xdebug from test/hotspot/jtreg
  • ... and 35 more: https://git.openjdk.org/jdk/compare/8042a50b99a671390910afa5f816894f77255429...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jul 27, 2023
@RogerRiggs
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 27, 2023

Going to push as commit 8650026.
Since your change was applied there have been 55 commits pushed to the master branch:

  • 25058cd: 8312620: WSL Linux build crashes after JDK-8310233
  • 8661b8e: 8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms
  • 486c784: 8312433: HttpClient request fails due to connection being considered idle and closed
  • 271417a: 8312579: [JVMCI] JVMCI support for virtual Vector API objects
  • 44576a7: 8312466: /bin/nm usage in AIX makes needs -X64 flag
  • 86821a7: 8312235: [JVMCI] ConstantPool should not force eager resolution
  • 7cbab1f: 8312218: Print additional debug information when hitting assert(in_hash)
  • 01e135c: 8312440: assert(cast != nullptr) failed: must have added a cast to pin the node
  • b7545a6: 8313164: src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels adjust releasing of resources
  • 36d578c: 8311653: Modify -XshowSettings launcher behavior
  • ... and 45 more: https://git.openjdk.org/jdk/compare/8042a50b99a671390910afa5f816894f77255429...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 27, 2023
@openjdk openjdk bot closed this Jul 27, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 27, 2023
@openjdk
Copy link

openjdk bot commented Jul 27, 2023

@RogerRiggs Pushed as commit 8650026.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@RogerRiggs RogerRiggs deleted the 8310033-time-compareto branch December 11, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
9 participants