On Thu, 5 Jan 2023 13:24:08 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this doc only change which addresses the >> javadoc issue noted in https://bugs.openjdk.org/browse/JDK-8258776? >> >> As noted in that issue, the `ThreadLocal.initialValue()` API javadoc >> suggests subclassing `ThreadLocal` and overriding the `initialValue()` >> method instead of recommending the `withInitial` method that was added in >> Java 8. >> >> The commit here updates the javadoc of `initialValue()` method to note that >> `withInitial` method is available for use if the programmer wants to provide >> an initial value. The updated javadoc continues to note that the >> `ThreadLocal` can be subclassed and the method overriden as the other way of >> providing an initial value. This change now uses the `@implSpec` construct >> to state this default behaviour of the `initialValue()` method. >> >> Additionally, the `get()` method's javadoc has also been updated to account >> for the semantics of `initialValue()`. In fact, in its current form, before >> the changes in this PR, the `get()` method states: >> >>> If the current thread does not support thread locals then >>> this method returns its {@link #initialValue} (or {@code null} >>> if the {@code initialValue} method is not overridden). >> >> which isn't accurate, since the `get()` will return a non-null value if the >> ThreadLocal was constructed through `ThreadLocal.withInitial`. Here's a >> trivial code which contradicts this current javadoc: >> >> >> public class Test { >> public static void main(final String[] args) throws Exception { >> Thread.ofPlatform().name("hello").allowSetThreadLocals(false).start(() >> -> { >> var t = ThreadLocal.withInitial(() -> 2); >> System.out.println("Thread local value is " + t.get()); >> }); >> } >> } >> >> Running this with `java --enable-preview --source 21 Test.java` returns: >> >> Thread local value is 2 >> >> >> The updated javadoc of `get()` in this PR now matches the behaviour of this >> method. I believe this will need a CSR, which I'll open once we have an >> agreement on these changes. >> >> Furthermore, the class level javadoc of `ThreadLocal` has an example of >> providing an initial value which uses the subclassing strategy. I haven't >> updated that part. Should we update that to show the `withInitialValue` >> usage instead (and maybe convert it to a snippet)? > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Review suggestion The CSR has been approved. Thank you Alan for the review. ------------- PR: https://git.openjdk.org/jdk/pull/11846
