On Tue, 15 Apr 2025 13:25:41 GMT, Viktor Klang <vkl...@openjdk.org> wrote:
>> Alan Bateman has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 15 commits: >> >> - Add JEP number, update copyright headers >> - Merge branch 'master' into JDK-8342486 >> - Sync up from loom repo >> - Merge branch 'master' into JDK-8342486 >> - Sync up from loom repo >> - Merge branch 'master' into JDK-8342486 >> - Merge branch 'master' into JDK-8342486 >> - Fix link >> - Merge branch 'master' into JDK-8342486 >> - Sync up impl/tests form loom repo >> - ... and 5 more: https://git.openjdk.org/jdk/compare/3090e218...418bc3d3 > > src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java > line 496: > >> 494: * processed. It should be clear what the {@code Joiner} does vs. >> the application >> 495: * code at the use-site. In general, the {@code Joiner} >> implementation is not the >> 496: * place to code "business logic". A {@code Joiner} should be >> designed to be as > > Suggestion: > > * place for "business logic". A {@code Joiner} should be designed to be > as Okay > src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java > line 563: > >> 561: * <p> In normal usage, this method will be called at most once >> by the {@code join} >> 562: * method to produce the result (or exception). The behavior of >> this method when >> 563: * invoked directly, and invoked more than once, is not >> specified. Where possible, > > Suggestion: > > * invoked directly, and invoked more than once, is undefined. Where > possible, Okay > src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java > line 778: > >> 776: /** >> 777: * {@return a new {@code Configuration} object with the given >> timeout} >> 778: * The other components are the same as this object. > > Suggestion: > > * The other other configuration options are preserved. The class introduces them as components of the configuration rather than "options" so I think I'll leave this as is. > src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java > line 806: > >> 804: * @param cause the cause, can be {@code null} >> 805: */ >> 806: public FailedException(Throwable cause) { > > Should this be public? That's a good question, there is no reason for these constructors to be public. > src/java.base/share/classes/java/util/concurrent/StructuredTaskScopeImpl.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > No changes in 2025? It was created in 2024, hasn't had any changes until the rename from Config -> Configuration. > src/java.base/share/classes/java/util/concurrent/StructuredTaskScopeImpl.java > line 424: > >> 422: * Used to schedule a task to cancel the scope when a timeout >> expires. >> 423: */ >> 424: private static class TimerSupport { > > Might make sense to use the commonPool():s DelayScheduler? TimerSupport hasn't changed, and pre-dates the FJP work to support delayed tasks, so maybe now is the time to migrate it. > test/jdk/java/util/concurrent/StructuredTaskScope/StructuredTaskScopeTest.java > line 1394: > >> 1392: assertEquals(2, subtasks.size()); >> 1393: assertTrue(subtasks.get(0) == subtask1); >> 1394: assertTrue(subtasks.get(1) == subtask2); > > Stylistic, there's an "assertSame" to assert that two references refer to the > same instance. Yes, indeed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046135423 PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046135526 PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046136931 PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046137297 PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046138621 PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046139774 PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2046144074
