RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed. ------------- Depends on: https://git.openjdk.java.net/jdk/pull/4073 Commit messages: - 8267521: Post JEP 411 refactoring: maximum covering > 50K Changes: https://git.openjdk.java.net/jdk/pull/4138/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267521 Stats: 226 lines in 18 files changed: 142 ins; 29 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/4138.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138 PR: https://git.openjdk.java.net/jdk/pull/4138
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: typo on windows ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/4138/files - new: https://git.openjdk.java.net/jdk/pull/4138/files/e3f0405a..d460efb8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4138.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138 PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 14:00:39 GMT, Weijun Wang <weijun@openjdk.org> wrote:
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
typo on windows
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
118: vals[1] = Integer.getInteger("sun.net.client.defaultConnectTimeout", 300_000).intValue(); 119: return System.getProperty("file.encoding", "ISO8859_1"); 120: }
It is a bit strange that "file.encoding" seem to get a special treatment - but I guess that's OK. src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
548: * @throws IOException if the connection was unsuccessful. 549: */ 550: @SuppressWarnings("removal")
Could the scope of the annotation be further reduced by applying it to the two places where `doPrivileged` is called in this method? src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 921:
919: } 920: 921: @SuppressWarnings("removal")
Could the scope be further reduced by applying it at line 926 in this method - at the cost of creating a temporary variable? The code could probably be further simplified by using a lambda... PrivilegedAction<Socket> pa = () -> new Socket(proxy); @SuppressWarnings("removal") var ps = AccessController.doPrivileged(pa); s = ps; ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
typo on windows
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
118: vals[1] = Integer.getInteger("sun.net.client.defaultConnectTimeout", 300_000).intValue(); 119: return System.getProperty("file.encoding", "ISO8859_1"); 120: }
It is a bit strange that "file.encoding" seem to get a special treatment - but I guess that's OK.
You might say we thus avoid wasting the return value, as much as possible. ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
typo on windows
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
548: * @throws IOException if the connection was unsuccessful. 549: */ 550: @SuppressWarnings("removal")
Could the scope of the annotation be further reduced by applying it to the two places where `doPrivileged` is called in this method?
I'll probably need to assign the doPriv result on L631 to a tmp variable and then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you already have similar suggestion in the next comment. ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update FtpClient.java ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/4138/files - new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=01-02 Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/4138.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138 PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang <weijun@openjdk.org> wrote:
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
update FtpClient.java
Thanks for taking in my suggestions for FtpClient. I have also reviewed the changes to java.util.logging and JMX. I wish there was a way to handle doPrivileged returning void more nicely. Maybe component maintainers will be able to deal with them on a case-by-case basis later on. ------------- Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4138
On Mon, 24 May 2021 09:00:07 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
Thanks for taking in my suggestions for FtpClient. I have also reviewed the changes to java.util.logging and JMX. I wish there was a way to handle doPrivileged returning void more nicely. Maybe component maintainers will be able to deal with them on a case-by-case basis later on.
Assigning to a useless "dummy" variable looks ugly. Extracting the call to a method might make it faraway from its caller. In L114 of `FtpClient.java` I managed to invent a return value and I thought it was perfect. But you said it's "a bit strange". :-( ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang <weijun@openjdk.org> wrote:
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value if not void. The local variable will be called `tmp` if later reassigned or `dummy` if it will be discarded. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
I'll add a copyright year update commit before integration.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
update FtpClient.java
src/java.desktop/share/classes/java/awt/Component.java line 630:
628: } 629: 630: @SuppressWarnings("removal")
I'm confused. I thought the reason this wasn't done in the JEP implementation PR is because of refactoring that was needed because of the usage in this static block and you could not apply the annotation here. Yet it seems you are doing exactly what was supposed to be impossible with no refactoring. Can you explain ? ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Thu, 27 May 2021 17:42:56 GMT, Phil Race <prr@openjdk.org> wrote:
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
update FtpClient.java
src/java.desktop/share/classes/java/awt/Component.java line 630:
628: } 629: 630: @SuppressWarnings("removal")
I'm confused. I thought the reason this wasn't done in the JEP implementation PR is because of refactoring that was needed because of the usage in this static block and you could not apply the annotation here. Yet it seems you are doing exactly what was supposed to be impossible with no refactoring. Can you explain ?
There *is* a tiny refactoring here: a new variable `s2` is introduced so the 2nd `doPrivileged` call on line 636 is now also in a declaration statement (for `s2`) and therefore annotatable. Without this I cannot add the annotation on line 635. ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 28 May 2021 02:50:55 GMT, Weijun Wang <weijun@openjdk.org> wrote:
src/java.desktop/share/classes/java/awt/Component.java line 630:
628: } 629: 630: @SuppressWarnings("removal")
I'm confused. I thought the reason this wasn't done in the JEP implementation PR is because of refactoring that was needed because of the usage in this static block and you could not apply the annotation here. Yet it seems you are doing exactly what was supposed to be impossible with no refactoring. Can you explain ?
There *is* a tiny refactoring here: a new variable `s2` is introduced so the 2nd `doPrivileged` call on line 636 is now also in a declaration statement (for `s2`) and therefore annotatable. Without this I cannot add the annotation on line 635.
Ok. But I will quote you "This happens when a deprecated method is called inside a static block. The annotation can only be added to a declaration and here it must be the whole class" So the way you explained this before made it sound like any time there was any SM API usage in a static block, the entire class needed to be annotated. Why has the explanation changed ? ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 28 May 2021 03:12:35 GMT, Phil Race <prr@openjdk.org> wrote:
There *is* a tiny refactoring here: a new variable `s2` is introduced so the 2nd `doPrivileged` call on line 636 is now also in a declaration statement (for `s2`) and therefore annotatable. Without this I cannot add the annotation on line 635.
Ok. But I will quote you "This happens when a deprecated method is called inside a static block. The annotation can only be added to a declaration and here it must be the whole class"
So the way you explained this before made it sound like any time there was any SM API usage in a static block, the entire class needed to be annotated.
Why has the explanation changed ?
I should have been more precise. An annotation can only be added on a declaration, whether it's a variable, a method, or a class. Static block is not a declaration and the only one covers it is the class. But then if it's on a local variable declaration inside a static block, we certainly can annotate on that variable. ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang <weijun@openjdk.org> wrote:
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value if not void. The local variable will be called `tmp` if later reassigned or `dummy` if it will be discarded. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
I'll add a copyright year update commit before integration.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
update FtpClient.java
Marked as reviewed by prr (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang <weijun@openjdk.org> wrote:
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value if not void. The local variable will be called `tmp` if later reassigned or `dummy` if it will be discarded. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
I'll add a copyright year update commit before integration.
Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
update FtpClient.java
The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8266459 git fetch https://git.openjdk.java.net/jdk master git merge FETCH_HEAD # if there are conflicts, follow the instructions given by git merge git commit -m "Merge master" git push ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value if not void. The local variable will be called `tmp` if later reassigned or `dummy` if it will be discarded. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
I'll add a copyright year update commit before integration.
Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: 8267521: Post JEP 411 refactoring: maximum covering > 50K ------------- Changes: https://git.openjdk.java.net/jdk/pull/4138/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=03 Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod Patch: https://git.openjdk.java.net/jdk/pull/4138.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138 PR: https://git.openjdk.java.net/jdk/pull/4138
On Fri, 21 May 2021 01:52:27 GMT, Weijun Wang <weijun@openjdk.org> wrote:
The code change refactors classes that have a `SuppressWarnings("removal")` annotation that covers more than 50KB of code. The big annotation is often quite faraway from the actual deprecated API usage it is suppressing, and with the annotation covering too big a portion it's easy to call other deprecated methods without being noticed.
The code change shows some common solutions to avoid such too wide annotations:
1. Extract calls into a method and add annotation on that method 2. Assign the return value of a deprecated method call to a new local variable and add annotation on the declaration, and then assign that value to the original l-value if not void. The local variable will be called `tmp` if later reassigned or `dummy` if it will be discarded. 3. Put declaration and assignment into a single statement if possible. 4. Rewrite code to achieve #3 above.
I'll add a copyright year update commit before integration.
This pull request has now been integrated. Changeset: 508cec75 Author: Weijun Wang <weijun@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/508cec7535cd0ad015d566389bc9e5f53ce4... Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod 8267521: Post JEP 411 refactoring: maximum covering > 50K Reviewed-by: dfuchs, prr ------------- PR: https://git.openjdk.java.net/jdk/pull/4138
participants (4)
-
unknown＠example.com
-
Daniel Fuchs
-
Phil Race
-
Weijun Wang