Re: RFR: 8339803: Acknowledge case insensitive unambiguous keywords in tzdata files [v2]
On Mon, 16 Sep 2024 19:30:56 GMT, Naoto Sato wrote:
>> make/jdk/src/classes/build/tools/tzdb/TzdbZoneRulesProvider.java line 308:
>>
>>> 306: if (off < tokens.length) {
>>> 307: String dayRule = tokens[off++];
>>> 308: if (dayRule.regionMatches(true, 0, "last", 0, 4)) {
>>
>> This isn't correct, as per the mailing list:
>>
>>> > can "last" contain uppercase letters, or
>>> > does it have to be exactly "last"?
>>>
>>> In that case the name is "lastSunday", and it can be abbreviated
>>> "lastSu" or "Lastsu" or "LASTSUNDA" or whatever.
>
> Why is it not? IIUC, abbreviation should be unambiguous, so "last" part is
> always case-insensitive 4 character length.
Sorry, I assumed this regionMatches was the same as the others. "last" is
always "last" and never abbreviated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20940#discussion_r1761774912
Re: RFR: 8339803: Acknowledge case insensitive unambiguous keywords in tzdata files [v2]
On Tue, 10 Sep 2024 22:42:37 GMT, Naoto Sato wrote:
>> This is a follow on fix to
>> [JDK-8339644](https://bugs.openjdk.org/browse/JDK-8339644). It turned out
>> that TZ data files allow abbreviation of keywords, such as "ZO" for "Zone."
>> Same fix, i.e, replacing `startsWith()` with `regionMatches()` was applied.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> tz files aligned with the default TzdbZoneRulesProvider list
make/jdk/src/classes/build/tools/tzdb/TzdbZoneRulesProvider.java line 308:
> 306: if (off < tokens.length) {
> 307: String dayRule = tokens[off++];
> 308: if (dayRule.regionMatches(true, 0, "last", 0, 4)) {
This isn't correct, as per the mailing list:
> > can "last" contain uppercase letters, or
> > does it have to be exactly "last"?
>
> In that case the name is "lastSunday", and it can be abbreviated
> "lastSu" or "Lastsu" or "LASTSUNDA" or whatever.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20940#discussion_r1761697764
Re: RFR: 8337279: Optimize format instant [v7]
On Tue, 27 Aug 2024 23:49:50 GMT, Shaojin Wen wrote: >> By removing the redundant code logic in >> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be >> reduced and the performance can be improved. > > Shaojin Wen 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 nine additional > commits since the last revision: > > - Speed up Instant.toString using JavaTimeAccess > - add JavaTimeAccess SharedSecrets > - Merge remote-tracking branch 'upstream/master' into > optim_instant_fmt_202407 > - breaking out the printNano methods > - copyright > - Update > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > >Co-authored-by: Stephen Colebourne > - 1. fix handle fraction == -1 >2. Split two methods to make codeSize less than 325 > - add comment > - optimize format instant The change looks OK from my perspective, however I would want someone with experience of `SharedSecrets` to comment - PR Review: https://git.openjdk.org/jdk/pull/20353#pullrequestreview-2273791076
Re: Package.getPackage deprecation
On Sun, 11 Aug 2024 at 17:19, Alan Bateman wrote: > Package.getPackage is deprecated a long time, I don't think we've seen too > many complaints. Nowadays it's probably not too useful except to get to > package annotations (everything else in that API dates from JDK 1.2 and the > since removed extension mechanism). > > The set of package names for packages in the modules defined to the boot > loader can be obtained with code like this: > > ModuleLayer.boot() > .modules() > .stream() > .filter(m -> m.getClassLoader() == null) > .flatMap(m -> m.getPackages().stream()) > .collect(Collectors.toSet()); > > which I think is what you are looking for here. That doesn't quite work as it returns String package names, not Package objects (I need the Package object to maintain compatibility). Sounds like Package.getPackages() is the only current workaround. FWIW I do think there is a case to review the deprecation text of Package.getPackage(String). thanks Stephen
Package.getPackage deprecation
I make use of Package.getPackage in Joda-Convert but the method has
now been deprecated. I'd like to update my code, but AFAICT the
suggested alternative does not work.
The Joda-Convert code allows a user to convert a Package object to a
String, and back again. Reading the deprecation, I'm aware that this
may not work perfectly, but I do have to maintain compatibility as
best I can.
The deprecation asks users to use ClassLoader#getDefinedPackage
instead. To do this properly, users have to loop up the ClassLoader
parent list. However, the boot class loader is generally returned as
null, and it is obviously impossible to call getDefinedPackage on a
null reference.
ie.
Package.getPackage("java.util") works OK, but is deprecated
The suggested replacement is this code, but it does not work because
the boot class loader is returned as null:
private static Package parsePackage(String str) {
var loader = JDKStringConverter.class.getClassLoader();
var pkg = (Package) null;
while (loader != null && pkg == null) {
pkg = loader.getDefinedPackage(str);
loader = loader.getParent();
}
return pkg;
}
Am I misunderstanding things?
See also
https://stackoverflow.com/questions/77259364/how-to-get-a-package-from-its-path-in-java
My current workaround is to call Package.getAllPackages() and search
for "java.util" manually, which appears to work OK.
thanks
Stephen
Re: RFR: 8337832: Optimize datetime toString
On Sat, 27 Jul 2024 13:45:11 GMT, Shaojin Wen wrote: > Similar to PR #20321, this improves performance by providing a method that > passes in a StringBuilder to avoid unnecessary object allocation. Change looks fine, but is it actually faster? - Marked as reviewed by scolebourne (Author). PR Review: https://git.openjdk.org/jdk/pull/20368#pullrequestreview-2231785832
Re: RFR: 8337279: Optimize format instant [v4]
On Sun, 11 Aug 2024 10:13:12 GMT, Shaojin Wen wrote: >> By removing the redundant code logic in >> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be >> reduced and the performance can be improved. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > > Co-authored-by: Stephen Colebourne A more productive direction might be to move `LocalDate.formatTo` and `LocalTime.formatTo` to a JDK internal class (eg. in `jdk.internal.util`?), and then edit `LocalTime.formatTo` to handle `fractionalDigits` directly as another method parameter. This would allow the formatters in `DateTimeFormatterBuilder` to directly use the `formatTo` methods without adding any new public APIs. - PR Comment: https://git.openjdk.org/jdk/pull/20353#issuecomment-2282706467
Re: RFR: 8337279: Optimize format instant [v3]
On Sun, 28 Jul 2024 15:52:03 GMT, Shaojin Wen wrote:
>> By removing the redundant code logic in
>> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be
>> reduced and the performance can be improved.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. fix handle fraction == -1
> 2. Split two methods to make codeSize less than 325
I'm still skeptical of some parts of this PR as it makes the code harder to
folow. The new tests are a good addition and should be merged.
Have you tried the performance of simply breaking out the
currentEra/beforeCurrentEra methods *without making any other changes*? Since
the logic to produce the nano fraction is going to stay in this method, I don't
really see the advantage in trying to get LocalDateTime to do the fraction some
of the time.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3818:
> 3816: if (fractionalDigits == 0) {
> 3817: inNano = 0;
> 3818: }
Suggestion:
if (fractionalDigits == 0) {
inNano = 0;
}
boolean printNanoInLocalDateTime = fractionalDigits == -2
|| (inNano == 0 && (fractionalDigits == 0 ||
fractionalDigits == -1));
-
PR Review: https://git.openjdk.org/jdk/pull/20353#pullrequestreview-2231779313
PR Review Comment: https://git.openjdk.org/jdk/pull/20353#discussion_r1712950065
Re: RFR: 8337838: Remove unused methods from ChronoLocalDateImpl
On Fri, 19 Jul 2024 09:52:50 GMT, Andrey Turbanov wrote:
> A few methods in `java.time.chrono.ChronoLocalDateImpl` are unused and could
> be removed:
> 1. plusWeeks(long)
> 2. minusYears(long)
> 3. minusMonths(long)
> 4. minusWeeks(long)
> 5. minusDays(long)
>
> Tested `test/jdk/java/time` on Linux x64 release
I believe the rest of the methods are OK to remove given the current design
src/java.base/share/classes/java/time/chrono/ChronoLocalDateImpl.java line 276:
> 274: * @throws DateTimeException if the result exceeds the supported
> date range
> 275: */
> 276: D plusWeeks(long weeksToAdd) {
Rather than removing it, this method should in fact be called from `plus(long
amountToAdd, TemporalUnit unit)` to give subclasses the chance to override the
7 day behaviour.
(Sure, this is an internal class, and there is no implementation that needs to
do that, but it is how the class was designed, and it would be clearer to fix
tha current mistaken setup)
-
PR Review: https://git.openjdk.org/jdk/pull/20250#pullrequestreview-2223469992
PR Review Comment: https://git.openjdk.org/jdk/pull/20250#discussion_r1706638887
Re: RFR: 8337279: Optimize format instant [v2]
On Fri, 26 Jul 2024 15:32:18 GMT, Shaojin Wen wrote:
>> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
>> line 3818:
>>
>>> 3816: }
>>> 3817: // add fraction
>>> 3818: if (fractionalDigits > 0) {
>>
>> This breaks the logic. `fractionalDigits` can be negative in the block below
>
> If fractionalDigits < 0, printNano is implemented in LocalDateTime
`fractionalDigits == -2` is used to output 0, 3, 6 or 9 fractional digits as
needed. This can be handled by `LocalDateTime`.
`fractionalDigits == -1` is used to output as many fractional digits as
needed, from 0 to 9 digits. This is NOT handled by `LocalDateTime`.
Furthermore, if the `-2` branch is handled by `LocalDateTime` then the logic
below for `-2` is redundant.
If the tests did not fail as a result of this change, then I imagine the tests
need improving.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20353#discussion_r1694248869
Re: RFR: 8337279: Optimize format instant [v2]
On Fri, 26 Jul 2024 15:44:59 GMT, Shaojin Wen wrote: >> By removing the redundant code logic in >> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be >> reduced and the performance can be improved. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comment src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3812: > 3810: int inNano = NANO_OF_SECOND.checkValidIntValue(inNanos != > null ? inNanos : 0); > 3811: // use LocalDateTime.toString, If fractionalDigits < 0, > printNano is implemented in LocalDateTime > 3812: LocalDateTime ldt = LocalDateTime.ofEpochSecond(inSecs, > fractionalDigits >= 0 ? 0 : inNano, ZoneOffset.UTC); I believe this whole approach is flawed. The comment above says: `// use INSTANT_SECONDS, thus this code is not bound by Instant.MAX` which indicates that the instant value may not fit in `LocalDateTime`. You may be able to special case certain code paths to use `LocalDateTime`, but that makes things more complex, and not necessarily faster. - PR Review Comment: https://git.openjdk.org/jdk/pull/20353#discussion_r1694249198
Re: RFR: 8337279: Optimize format instant
On Fri, 26 Jul 2024 14:20:07 GMT, Shaojin Wen wrote:
> By removing the redundant code logic in
> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be
> reduced and the performance can be improved.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3818:
> 3816: }
> 3817: // add fraction
> 3818: if (fractionalDigits > 0) {
This breaks the logic. `fractionalDigits` can be negative in the block below
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20353#discussion_r1693253379
Re: RFR: 8337168: Optimize LocalDateTime.toString [v4]
On Thu, 25 Jul 2024 23:05:46 GMT, Shaojin Wen wrote: >> The current LocalDateTime.toString implementation concatenates the toString >> results of date and time, which can be passed to StringBuilder to reduce >> additional object allocation and improve performance. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/java/time/LocalTime.java > > Co-authored-by: Chen Liang This change is fine, however the comments and performance testing assume that a date is always 10 characters long which isn't true, as the year could be greater than . - PR Review: https://git.openjdk.org/jdk/pull/20321#pullrequestreview-2201711797
Re: java.time lacks start and end aware period data types
There are a variety of ways that a range of the timeline can be defined. And some would argue that Java should have a general purpose Range class to cover all Comparable classes rather than a few specific ones for Java time. Ultimately, Java time chose to avoid the issues by not addressing the design space. There were plenty of other things to tackle at the time. BTW, see ThreeTen-Extra for some range and interval classes. Stephen On Wed, 19 Jun 2024, 20:44 Olexandr Rotan, wrote: > Greetings to the Java community. I have a question regarding the design of > java,time package. > > Commercial Java developers deal with time periods all the time with > different validations regarding intersection of periods, big data > processing, entity auditing etc etc. And it is surprising for everyone to > look into java.time for the first time and find out that there is no start > and end aware span data types at all! > > There is no rocket science in implementing such data types yourself, > that's for sure. However, as always, there are hidden spikes in these > waters.To use these types in JPA entities, you would have to write a custom > converter to familiar for database type, which ties project tightly to > database, which is contrary to what JPA was intended for . To use it in > auditing. one would have to write a custom auditor as no framework or > library knows about our custom data type. Same goes for big data > processing: no library or framework would support processing series of your > custom data type, at least unless you provide some adapter using library > mechanisms, if those exist at all. This rant could go on and on, but the > points are: > > 1) Lack of standardization for such data types leads to obstacles on each > step of development: constant writing of adapters, converters and blah blah > blah. Each of those serves as a space to make mistakes. One-two easy > implementations are not likely to be the source of errors, but the more a > project grows, the more dependencies it acquires, the more it is likely to > fall for some subtle specific feature of some library and spend hours or > even days debugging. > 2) One could rightfully argue that for small projects or microservice > projects with no shared codebase and different team for each service, this > whole mess I described above is overengineering. Seriously, not everyone > would want to go through all of that for one-two usages of class in the > domain layer of the project, and, moreover, not everyone should. However, > leaving it be "as is" is a screaming source of data inconsistencies, as it > is now responsibility of developer (or even worse, user) to keep in mind > the requirement to update all fields, that combined form that start- and > end-aware period (like start date/datetime and Period/Duration as common > way to emulate such thing) > > So, the question is: was it a conscious decision to omit such types back > in the day when java.time was designed, or is it a blindspot created by > deadlines/lack of resources/something else? > > Would appreciate anything some of you have to share on this topic. > > Best regards >
Re: Instant.now(Clock) and InstantSource
On Mon, 3 Jun 2024 at 22:25, Kurt Alfred Kluever wrote: > It feels a bit strange that you can't pass an `InstantSource` to > `Instant.now(...)`, but you _can_ pass a `Clock` (which of course has a > "useless" `ZoneId` when creating an `Instant`). Therefore, I'd like to > propose one of the following API changes: > > 1) adding `Instant.now(InstantSource)` > 2) deprecating `Instant.now(Clock)` in favor of `clock.instant()` I have no problem with adding `Instant.now(InstantSource)`, but I think deprecating the `Clock` method is unnecessary (given it will never be removed AFAICT, and it is no doubt in widespread use). Stephen
Re: In support of Instant.minus(Instant)
stant2) seems >> >>> both less discoverable and less obvious. >> >>> >> >>> On Thu, 2 May 2024 at 10:29, Louis Wasserman > >>> <mailto:lowas...@google.com>> wrote: >> >>> >> >>> That doesn't follow for me at all. >> >>> >> >>> The structure formed by Instants and Durations is an affine space >> >>> <https://en.wikipedia.org/wiki/Affine_space#Definition>, with >> >>> instants the points and durations the vectors. (An affine space is >> >>> a vector space without a distinguished origin, which of course >> >>> Instants don't have.) It is 100% standard to use the minus sign for >> >>> the operation "point - point = vector," even when "point + point" is >> >>> not defined, and to use all the other standard idioms for >> >>> subtraction; the Wikipedia article uses "subtraction" and >> >>> "difference" ubiquitously. >> >>> >> >>> Personally, I'd be willing to live with a different name for the >> >>> operation, but consider "users keep getting it wrong" a strong >> >>> enough argument all by itself for a version with the swapped >> >>> argument order; it's not obvious to me that another API with the >> >>> same argument order adds enough value over Duration.between to >> >>> bother with. >> >>> >> >>> On Thu, May 2, 2024 at 10:04 AM Stephen Colebourne >> >>> mailto:scolebou...@joda.org>> wrote: >> >>> >> >>> On Thu, 2 May 2024 at 15:58, Kurt Alfred Kluever > >>> <mailto:k...@google.com>> wrote: >> >>> > instant − instant = duration // what we're discussing >> >>> > instant + duration = instant // satisfied by >> >>> instant.plus(duration) >> >>> > instant - duration = instant // satisfied by >> >>> instant.minus(duration) >> >>> > duration + duration = duration // satisfied by >> >>> duration.plus(duration) >> >>> > duration - duration = duration // satisfied by >> >>> duration.minus(duration) >> >>> > duration × real number = duration // satisfied by >> >>> duration.multipliedBy(long) >> >>> > duration ÷ real number = duration // satisfied by >> >>> duration.dividedBy(long) >> >>> > >> >>> > All but the first operation have very clear translations from >> >>> conceptual model to code. I'm hoping we can achieve the same >> >>> clarity for instant - instant by using the obvious name: >> >>> instant.minus(instant) >> >>> >> >>> But you can't have >> >>> instant + instant = ??? >> >>> It doesn't make sense. >> >>> >> >>> This is at the heart of why minus isn't right in this case. >> >>> Stephen >> >>> >> >>> >> >>> >> >>> -- Louis Wasserman (he/they) >> >>> >> > > > > > -- > kak
Re: In support of Instant.minus(Instant)
On Thu, 2 May 2024 at 15:58, Kurt Alfred Kluever wrote: > instant − instant = duration // what we're discussing > instant + duration = instant // satisfied by instant.plus(duration) > instant - duration = instant // satisfied by instant.minus(duration) > duration + duration = duration // satisfied by duration.plus(duration) > duration - duration = duration // satisfied by duration.minus(duration) > duration × real number = duration // satisfied by duration.multipliedBy(long) > duration ÷ real number = duration // satisfied by duration.dividedBy(long) > > All but the first operation have very clear translations from conceptual > model to code. I'm hoping we can achieve the same clarity for instant - > instant by using the obvious name: instant.minus(instant) But you can't have instant + instant = ??? It doesn't make sense. This is at the heart of why minus isn't right in this case. Stephen
Re: RFR: 8331202: Support for Duration until another Instant [v2]
On Mon, 29 Apr 2024 21:32:16 GMT, Naoto Sato wrote: >> A new method on Instant to return the duration `until` another Instant is >> suggested per the following discussion thread: >> >> https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html >> >> A CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Test with Duration.between() Marked as reviewed by scolebourne (Author). - PR Review: https://git.openjdk.org/jdk/pull/19007#pullrequestreview-2030326257
Re: In support of Instant.minus(Instant)
java.time.* already has the `until(ChronoLocalDate)` method on LocalDate. It would be reasonable to add a similar method to various other classes. This potentially gives you Duration dur = start.until(end) I'm wary of adding the opposite (given until() is already there). I'm particularly wary of using minus() as the verb for the opposite as minus() means something different in other parts of the API (minus() is used to subtract a TemporalAmounrt, not a Temporal). Stephen On Thu, 25 Apr 2024 at 19:57, Kurt Alfred Kluever wrote: > > Hi core-libs-dev, > > The java.time API already supports subtracting two Instants (end - start) via > Duration.between(Temporal, Temporal), but we've found the parameter ordering > (which requires a bit of "mental gymnastics") and API location to be a bit > unnatural. > > Parameter Ordering > > We very often see folks write code like this: Duration elapsed = > Duration.ofMillis(end.toEpochMilli() - start.toEpochMilli()); > > This closely matches the mental model of what they're trying to accomplish, > but it is longer (and more complex) than it needs to be, it drops > sub-millisecond precision, and it requires decomposing the java.time types > (which we strongly discourage). If you want to "simplify" the above > statement, you must remember to swap the argument order: Duration elapsed = > Duration.between(start, end); > > Many of us find representing (end - start) as between(start, end) to be > confusing. > > API Location > > We do not believe Duration is the most obvious place to find this method; if > you want a way to subtract two Instant values, an instance method on Instant > is a more obvious place to look. Pushing what could be an instance method to > a static utility method feels unnatural. > > JDK-8276624 (https://bugs.openjdk.org/browse/JDK-8276624) proposes to add > Temporal.minus(Temporal) as a default method (which would effectively > accomplish the same thing), but we do not recommend implementing that > proposal as specified. A default method on Temporal would require runtime > exceptions to be thrown from other Temporal types like LocalDate or Year. It > would also allow oddities like instant.minus(year) to compile (but presumably > throw at runtime). Conceptually, this API would not make sense for certain > types (e.g., LocalDate — the difference between two LocalDates is a Period, > not a Duration). > > Instead, we recommend adding a new instance method: instant.minus(instant) > (which returns a Duration), and possibly also adding > localDate.minus(localDate) (which returns a Period). However note that we've > seen a lot of confusion using the Period API (but that's a separate > discussion). > > While we generally don't like having 2 public APIs that accomplish the same > thing, in this case we feel the discoverability and simplicity of the new > API(s) outweighs the cost of an additional public API. > > Please consider this a +1 from our team to add instant.minus(instant). > > Regards, > > -Kurt Alfred Kluever (k...@google.com) > (on behalf of Google's Java and Kotlin Ecosystem Team, aka the Guava team)
Re: RFR: 8313231: Redundant if statement in ZoneInfoFile
On Thu, 27 Jul 2023 06:46:46 GMT, John Jiang wrote:
> if (i < savingsInstantTransitions.length) {
> // javazic writes the last GMT offset into index 0!
> if (i < savingsInstantTransitions.length) {
> offsets[0] = standardOffsets[standardOffsets.length - 1] * 1000;
> nOffsets = 1;
> }
> ...
> }
>
>
> The second if statement looks unnecessary.
Marked as reviewed by scolebourne (Author).
-
PR Review: https://git.openjdk.org/jdk/pull/15052#pullrequestreview-1554092886
Re: RFR: 8313231: Redundant if statement in ZoneInfoFile
On Thu, 27 Jul 2023 11:01:23 GMT, Pavel Rappo wrote:
>> if (i < savingsInstantTransitions.length) {
>> // javazic writes the last GMT offset into index 0!
>> if (i < savingsInstantTransitions.length) {
>> offsets[0] = standardOffsets[standardOffsets.length - 1] * 1000;
>> nOffsets = 1;
>> }
>> ...
>> }
>>
>>
>> The second if statement looks unnecessary.
>
> src/java.base/share/classes/sun/util/calendar/ZoneInfoFile.java line 471:
>
>> 469: if (i < savingsInstantTransitions.length) {
>> 470: // javazic writes the last GMT offset into index 0!
>> 471: if (i < savingsInstantTransitions.length) {
>
> Even my IDE flags it as always true. While it surely is redundant, I wonder
> if some other check was intended instead. @jodastephen?
I'm not sure this is my code - I certainly don't remember it. This PR causes no
effective change but be slightly neater, so I guess I approve the PR
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15052#discussion_r1278874277
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v9]
On Fri, 21 Jul 2023 15:59:42 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 14 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8310033-time-compareto
> - Corrected descriptions with respect to chronology and the concrete
> temporary type.
> - Improve wording and remove markup for clarity
> - Merge branch 'master' into 8310033-time-compareto
> - Correct the descriptions to correctly identify the compareTo return
>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.
> - Improve the grammar of "the comparator value is" -> "the comparator value,
> that is"
>Thanks for the reminder.
> - Merge branch 'master' into 8310033-time-compareto
> - Improve descriptions to be more specific and remove inappropriate use of
> before/after
>Remove extra blank lines
> - Clarify return values of date time classes
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - ... and 4 more: https://git.openjdk.org/jdk/compare/f4b0e559...5ecd7aeb
LGTM
-
Marked as reviewed by scolebourne (Author).
PR Review: https://git.openjdk.org/jdk/pull/14479#pullrequestreview-1541440889
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v7]
On Tue, 11 Jul 2023 17:54:23 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 12 additional
> commits since the last revision:
>
> - Improve wording and remove markup for clarity
> - Merge branch 'master' into 8310033-time-compareto
> - Correct the descriptions to correctly identify the compareTo return
>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.
> - Improve the grammar of "the comparator value is" -> "the comparator value,
> that is"
>Thanks for the reminder.
> - Merge branch 'master' into 8310033-time-compareto
> - Improve descriptions to be more specific and remove inappropriate use of
> before/after
>Remove extra blank lines
> - Clarify return values of date time classes
> - 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
> - ... and 2 more: https://git.openjdk.org/jdk/compare/cbfb208d...1d39e2d4
Changes requested by scolebourne (Author).
src/java.base/share/classes/java/time/LocalDate.java line 1991:
> 1989: *
> 1990: * @param other the other date to compare to, not null
> 1991: * @return the comparator value, that is less than zero if this is
> before {@code other},
This text is incorrect. It does not match the description above re chronologies.
src/java.base/share/classes/java/time/LocalDateTime.java line 1810:
> 1808: *
> 1809: * @param other the other date-time to compare to, not null
> 1810: * @return the comparator value, that is less than zero if this is
> before {@code other},
This text is incorrect. It does not match the description above re chronologies.
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, that is the comparison with the
> {@code other}'s instant, if they are not equal;
This text is incorrect. It does not match the description above re offsets.
(This would be a suitable description for `compareInstant`)
src/java.base/share/classes/java/time/OffsetTime.java line 1286:
> 1284: * @return the comparator value, that is the comparison of the UTC
> equivalent {@code other} instant,
> 1285: * if they are not equal, and if the UTC equivalent {@code
> other} instant is equal,
> 1286: * the comparison of this local date-time with {@code
> other} local date-time
This text is incorrect. It compares the local time, not the local date-time,
when the instants are equal
src/java.base/share/classes/java/time/chrono/ChronoLocalDate.java line 703:
> 701: *
> 702: * @param other the other date to compare to, not null
> 703: * @return the comparator value, that is less than zero if this is
> before {@code other},
This text is incorrect. It does not match the description above re chronologies.
-
PR Review: https://git.openjdk.org/jdk/pull/14479#pullrequestreview-1539950887
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1269880032
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1269879973
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1269881117
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1269883364
PR Review Comment: https://git.openjdk.org/jdk/pull/14479#discussion_r1269884657
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: 8307466: java.time.Instant calculation bug in until and between methods
On Fri, 5 May 2023 21:28:25 GMT, Roger Riggs wrote: > The implementation of java.time.Instant.until(I2, ChronoUnit) in some cases > did not correctly borrow or carry from the nanos to the seconds when > computing using ChronoUnit.MILLIS or ChronoUnit.MICROS. > The errant computation was introduced by > [JDK-8273369](https://bugs.openjdk.org/browse/JDK-8273369). Marked as reviewed by scolebourne (Author). - PR Review: https://git.openjdk.org/jdk/pull/13846#pullrequestreview-1415731468
Re: Time difference calculation bug
This looks like a bug from the description below, although it is surprising
that it isn't caught by tests.
Stephen
On Wed, 3 May 2023, 21:47 SHARPE, Stuart (Commercial & Institutional CDIO),
wrote:
> Hi Roger,
>
> Thanks for the reply.
>
> I must admit I am struggling to understand your explanation. I can't see
> how the choice of internal representation would affect the result in this
> way. I also don't see anything in the JavaDoc that describes values being
> truncated prior to the calculation, and even if it did, the results are not
> consistent - if that was the expected behaviour, the results from my
> previous example would have been 1000 and 1001, would they not?
>
> The part I quoted from the JavaDoc seems quite clear: it should return the
> number of whole units (millis in this case) between the two temporals. The
> number of milliseconds between 10:00:00.000999 and 10:00:01.000 is 999.001,
> which is 999 whole units.
>
> I think the below illustration makes it clearer that there is something
> wrong. The output values are all what I would expect as per the JavaDoc
> apart from the result for t2 = 10:00:01.000.
>
> var t1 = Instant.parse("2023-05-03T10:00:00.000999Z");
> var t2 = Instant.parse("2023-05-03T10:00:00.995Z");
>
> for(int i=0; i<10; i++) {
> System.out.println(ChronoUnit.MILLIS.between(t1, t2));
> t2 = t2.plusMillis(1);
> }
>
> This prints:
>
> 994
> 995
> 996
> 997
> 998
> 1000
> 1000
> 1001
> 1002
> 1003
>
> I'm fully prepared to accept that I've missed something, but at the moment
> I can't understand what that is.
>
> Regards,
>
> Stuart
>
>
> From: core-libs-dev On Behalf Of Roger
> Riggs
> Sent: 03 May 2023 20:58
> To: core-libs-dev@openjdk.org
> Subject: Re: Time difference calculation bug
>
>
> Hi,
>
> The seconds and nano-seconds are computed separately.
> The representation of Instant holds seconds and nanoseconds separately.
>
> The computation is performed on the nano-seconds of the Instant and
> truncated to milliseconds.
> The value of 0.000999 becomes 0 when truncated to MILLIS.
>
> Regards, Roger
> On 5/3/23 12:54 PM, SHARPE, Stuart (Commercial & Institutional CDIO) wrote:
> Hello,
>
> I believe I have discovered a bug in java.time. I've searched Jira and
> couldn't find any existing similar issue.
>
> Consider the following code:
>
> var t1 = Instant.parse("2023-05-03T10:00:00.000999Z");
> var t2 = Instant.parse("2023-05-03T10:00:01.000Z");
> var t3 = Instant.parse("2023-05-03T10:00:01.001Z");
>
> System.out.println(MILLIS.between(t1, t2));
> System.out.println(MILLIS.between(t1, t3));
>
> This prints:
>
> 1000
> 1000
>
> Given that t3 is exactly one millisecond later than t2, it does not seem
> reasonable that they can both be 1000 milliseconds later than t1.
>
> The JavaDoc for between() states "The calculation returns a whole number,
> representing the number of complete units between the two temporals."
>
> Assuming I have understood this correctly, I think MILLIS.between(t1, t3)
> is correct but MILLIS.between(t1, t2) should return 999.
>
> Regards
>
> Stuart Sharpe
>
>
>
> This communication and any attachments are confidential and intended
> solely for the addressee. If you are not the intended recipient please
> advise us immediately and delete it. Unless specifically stated in the
> message or otherwise indicated, you may not duplicate, redistribute or
> forward this message and any attachments are not intended for distribution
> to, or use by any person or entity in any jurisdiction or country where
> such distribution or use would be contrary to local law or regulation.
> NatWest Markets Plc, NatWest Markets N.V., NatWest Markets Securities Japan
> Limited and/or NatWest Markets Securities Inc. (collectively "NatWest
> Markets") accepts no responsibility for any changes made to this message
> after it was sent.
>
> This communication, where prepared by the sales and trading desk or desk
> strategists, may be marketing material, desk strategy and/or trader
> commentary. It is not a product of the research department. This material
> may constitute an invitation to consider entering into a derivatives
> transaction under U.S. CFTC Regulations sections 1.71 and 23.605, where
> applicable, but is not a binding offer to buy/sell any financial
> instrument. The views of the author may differ from others at NatWest
> Markets.
>
> Unless otherwise specifically indicated, the contents of this
> communication and its attachments are for information purposes only and
> should not be regarded as an offer or solicitation to buy or sell a product
> or service, confirmation of any transaction, a valuation, indicative price
> or an official statement. Trading desks may have a position or interest
> that is inconsistent with any views expressed in this message. In
> evaluating the information contained in this message, you should know that
> it could have been previously provided to other clients and/or internal
>
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset [v4]
On Wed, 1 Feb 2023 20:19:20 GMT, cheenar wrote:
>> test/jdk/java/time/tck/java/time/zone/TCKFixedZoneRules.java line 141:
>>
>>> 139: @Test(dataProvider="rules")
>>> 140: public void test_isValidOffset_LDT_ZO(ZoneRules test, ZoneOffset
>>> expectedOffset) {
>>> 141: if (expectedOffset == ZoneOffset.UTC)
>>
>> Extremely minor but why not wrap if with `{}` for improved readability here
>> with the comment
>
> Same
> [here](https://github.com/openjdk/jdk/pull/12346/commits/ec49ca3bc03d2e97fa0429c8429092307871?diff=unified&w=0#diff-9e5aa282dc2d02c31e1d7c5ec8196a1d3d23c06e471d5114d0bd0c78ee4fe5f6R433)
> although it feels much more dangerous than the test!
Yes, please always wrap with `{}` in java.time.*
-
PR: https://git.openjdk.org/jdk/pull/12346
