On Fri, 2 Feb 2024 23:36:41 GMT, Joe Darcy <da...@openjdk.org> wrote:
> After the "this-escape" lint warning was added to javac (JDK-8015831), the
> base module was not updated to be able to compile with this warning enabled.
> This PR makes the necessary changes to allow the base module to build with
> the warning enabled.
I looked at the modifications in java.net / sun.net. Looks generally good
though I have some comments.
src/java.base/share/classes/java/net/Socket.java line 323:
> 321: * @see SecurityManager#checkConnect
> 322: */
> 323: @SuppressWarnings("this-escape")
This is a weird one. I guess the issue here is that the escape happens in the
chained constructor, and is propagated recursively up the constructor chain. Is
the suppress warning here still needed after disabling this-escape warning at
line 358?
Actually - these are all weird since the only place where the escape happens is
in the private constructor at line 548 - and it doesn't even get flagged there
(presumably because it's a private constructor?)
I guess that the rationale is that subclasses cannot override the private
constructor (where the escape happen), but can override the public constructor
that calls the private constructor where the escape happen. I can't help
feeling that the warning would be better placed on the private constructor
though. Seeing it here confused me a lot.
src/java.base/share/classes/sun/net/www/MessageHeader.java line 53:
> 51: }
> 52:
> 53: @SuppressWarnings("this-escape")
An alternative here could be to make the class final. AFAICS it's not
subclassed anywhere. If you'd prefer not to do this here then maybe a followup
issue could be logged?
-------------
PR Review: https://git.openjdk.org/jdk/pull/17692#pullrequestreview-1865378355
PR Review Comment: https://git.openjdk.org/jdk/pull/17692#discussion_r1479922189
PR Review Comment: https://git.openjdk.org/jdk/pull/17692#discussion_r1479936447
