Re: RFR: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions [v2]
On Fri, 5 Apr 2024 12:42:04 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/net/ServerSocket.java line 264:
>>
>>> 262: * 0 and 65535, inclusive.
>>> 263: *
>>> 264: * @see StandardSocketOptions
>>
>> What would you think about dropping this link, and the link to SocketImpl
>> from all the constructors, they aren't relevant for anyone reading this part
>> of the docs.
>
> These constructors talk about the `{@code createSocketImpl} method`, so I'm
> guessing the `@see java.net.SocketImpl` was meant to provide reference to the
> `SocketImpl`. But I think we should just change the `{@code createSocketImpl}
> method` to `{@link SocketImplFactory#createSocketImpl()} method` and remove
> these `@see java.net.SocketImpl`.
>
> I guess we could do the same and remove the `@see java.net.SocketImpl` from
> the constructors of `java.net.Socket` too?
I've now updated the PR with what I had in mind for this proposed change to
these constructors.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18646#discussion_r1553592558
Re: RFR: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions [v2]
On Fri, 5 Apr 2024 12:19:13 GMT, Alan Bateman wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove repeated usages of {@link}
>
> src/java.base/share/classes/java/net/ServerSocket.java line 264:
>
>> 262: * 0 and 65535, inclusive.
>> 263: *
>> 264: * @see StandardSocketOptions
>
> What would you think about dropping this link, and the link to SocketImpl
> from all the constructors, they aren't relevant for anyone reading this part
> of the docs.
These constructors talk about the `{@code createSocketImpl} method`, so I'm
guessing the `@see java.net.SocketImpl` was meant to provide reference to the
`SocketImpl`. But I think we should just change the `{@code createSocketImpl}
method` to `{@link SocketImplFactory#createSocketImpl()} method` and remove
these `@see java.net.SocketImpl`.
I guess we could do the same and remove the `@see java.net.SocketImpl` from the
constructors of `java.net.Socket` too?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18646#discussion_r1553562060
Re: RFR: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions [v2]
On Fri, 5 Apr 2024 12:06:21 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this doc-only changes to java.net.ServerSocket
>> and java.net.Socket classes?
>>
>> As noted in https://bugs.openjdk.org/browse/JDK-8329745, these classes
>> currently refer to the legacy `java.net.SocketOptions` interface and instead
>> should be refering to the newer `java.net.StandardSocketOptions` class. The
>> commit in this PR updates such references. This change intentionally doesn't
>> do any code changes to use the `StandardSocketOptions` class - that can be
>> done separately if desired at a later point (after testing for any
>> compatibility issues). Finally, there are a few places in ServerSocket and
>> Socket documentation which will continue to refer to java.net.SocketOptions
>> legacy interface because few of the options aren't available in
>> StandardSocketOptions class (for example, `SO_TIMEOUT`).
>>
>> I ran `make docs-image` locally with this change and the generated doc looks
>> OK to me.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove repeated usages of {@link}
src/java.base/share/classes/java/net/ServerSocket.java line 264:
> 262: * 0 and 65535, inclusive.
> 263: *
> 264: * @see StandardSocketOptions
What would you think about dropping this link, and the link to SocketImpl from
all the constructors, they aren't relevant for anyone reading this part of the
docs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18646#discussion_r1553527158
Re: RFR: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions [v2]
> Can I please get a review of this doc-only changes to java.net.ServerSocket
> and java.net.Socket classes?
>
> As noted in https://bugs.openjdk.org/browse/JDK-8329745, these classes
> currently refer to the legacy `java.net.SocketOptions` interface and instead
> should be refering to the newer `java.net.StandardSocketOptions` class. The
> commit in this PR updates such references. This change intentionally doesn't
> do any code changes to use the `StandardSocketOptions` class - that can be
> done separately if desired at a later point (after testing for any
> compatibility issues). Finally, there are a few places in ServerSocket and
> Socket documentation which will continue to refer to java.net.SocketOptions
> legacy interface because few of the options aren't available in
> StandardSocketOptions class (for example, `SO_TIMEOUT`).
>
> I ran `make docs-image` locally with this change and the generated doc looks
> OK to me.
Jaikiran Pai has updated the pull request incrementally with one additional
commit since the last revision:
remove repeated usages of {@link}
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18646/files
- new: https://git.openjdk.org/jdk/pull/18646/files/0e393c88..46ece095
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18646=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18646=00-01
Stats: 50 lines in 2 files changed: 0 ins; 10 del; 40 mod
Patch: https://git.openjdk.org/jdk/pull/18646.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18646/head:pull/18646
PR: https://git.openjdk.org/jdk/pull/18646
Re: RFR: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions [v2]
On Fri, 5 Apr 2024 11:29:14 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/net/ServerSocket.java line 867:
>>
>>> 865: * setting of {@link StandardSocketOptions#SO_REUSEADDR
>>> SO_REUSEADDR}.
>>> 866: *
>>> 867: * The behaviour when {@link StandardSocketOptions#SO_REUSEADDR
>>> SO_REUSEADDR} is
>>
>> I suppose the main question here is whether the description really needs to
>> link to SO_REUSEADDR five times, it seems a bit excessive. In cases like
>> this I tend to just have the first usage link, others do it differently.
>
> Hello Alan, I too typically follow the process of linking once and then using
> `{@code SO_REUSEADDR}`. I let it stay in this form since it was already that
> way. I'll go ahead and update it to link only once.
Done - I've updated the PR to remove the repeated links.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18646#discussion_r1553507116
