On Tue, 26 Nov 2024 18:00:21 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Please review this PR which adds the `final` modifier to non-subclassable
>> classes in `java.base`.
>>
>> The classes were identified using an automated analysis. See CSR for details.
>>
>> Besides simply adding the `final` access modifier, the PR:
>>
>> * Updates a note in `java.lang.constant.DynamicCallSiteDesc` to not
>> reference subtypes. See CSR for discussion.
>> * Removes the class `java.lang.Runtime` from the test
>> `test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java`
>> * Updates the copyright year of affected source files
>
> Changes requested by liach (Reviewer).
Following some offline discussion with @liach, we decided to leave out the
constructor access updates in `ModuleDescriptor` and `InterfaceAddress` for
now. They are not considered a specification change and adds noise for the CSR
review.
These changes may be revisited in this PR pending CSR approval, or can be
addressed in follow-up PRs.
> src/java.base/share/classes/java/lang/constant/DynamicCallSiteDesc.java line
> 44:
>
>> 42: * {@code invokedynamic} call site.
>> 43: *
>> 44: * <p>This class is immutable and its behavior does not rely on object
>> identity
>
> Suggestion:
>
> * <p>A {@code DynamicCallSiteDesc} is immutable and its behavior does not
> * rely on object identity.
>
> This describes an object, not a class, and please close sentences with a
> period.
> (We use fragments for param and return block tags instead)
Thanks, fixed in 6156b46 :)
> src/java.base/share/classes/java/lang/constant/DynamicCallSiteDesc.java line
> 45:
>
>> 43: *
>> 44: * <p>Concrete subtypes of {@linkplain DynamicCallSiteDesc} should be
>> immutable
>> 45: * and their behavior should not rely on object identity.
>
> Please reword this to something like:
>
> {@code DynamicCallSiteDesc} is immutable and its behavior does not rely on
> object identity.
>
> This is given in `ConstantDesc`, but `DynamicCallSiteDesc` does not extend
> `ConstantDesc` so the removal is dubious.
Thanks, I've updated the PR and CSR to retain the note about immutability and
object identity, removing the reference to subtypes:
This class is immutable and its behavior does not rely on object identity
I opted to replace the self-reference `{@code DynamicCallSiteDesc}` with just
"This class" here. Let me know if you prefer spelling out the class name.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/22389#issuecomment-2501808953
PR Review Comment: https://git.openjdk.org/jdk/pull/22389#discussion_r1859123165
PR Review Comment: https://git.openjdk.org/jdk/pull/22389#discussion_r1859051463
