RFR: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place of null checks followed by NPE throws, mark private fields final where possible, use lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where possible, and generally sanitize/deduplicate. ------------- Commit messages: - Tidy - require non null in SimpleBindings - Simplify SimpleScriptContext.scopes - Mark fields final where possible - Deduplicate registerEngineXxx methods - Misc tidying - Deduplicate exception reporting - Lambdify - Mark fields as final; eliminate now unnecessary init() method. - Deduplicate engine creation and setup code - ... and 4 more: https://git.openjdk.java.net/jdk/compare/a209ed01...f378f350 Changes: https://git.openjdk.java.net/jdk/pull/3229/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3229&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264326 Stats: 225 lines in 5 files changed: 38 ins; 142 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/3229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3229/head:pull/3229 PR: https://git.openjdk.java.net/jdk/pull/3229
On Sat, 27 Mar 2021 15:19:37 GMT, Attila Szegedi <attila@openjdk.org> wrote:
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place of null checks followed by NPE throws, mark private fields final where possible, use lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where possible, and generally sanitize/deduplicate.
src/java.scripting/share/classes/javax/script/ScriptEngineManager.java line 224:
222: try { 223: List<String> matches = valuesFn.apply(spi); 224: return matches != null && matches.contains(selector);
Note this under anomalous conditions, calling `List.contains` here can result in somewhat different behavior than [what was here before](https://github.com/openjdk/jdk/blob/master/src/java.scripting/share/classes/...). Notably: * if `spi` returns a list that contains the selector value _at least twice_, and * it throws an exception while creating the engine for the first time, then * the previous implementation will invoke `getScriptEngine` for the second time too. My modified implementation will ignore multiple occurrences of the selector value in the returned list. I believe that behavior is correct, especially since the list of values for engine names, extensions, and mime types is expected to contain unique entries (too bad the interface didn't define these methods to return a `Set<String>`…) ------------- PR: https://git.openjdk.java.net/jdk/pull/3229
On Sat, 27 Mar 2021 15:19:37 GMT, Attila Szegedi <attila@openjdk.org> wrote:
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place of null checks followed by NPE throws, mark private fields final where possible, use lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where possible, and generally sanitize/deduplicate.
src/java.scripting/share/classes/javax/script/ScriptEngineManager.java line 215:
213: } 214: 215: private ScriptEngine getEngineBy(String selector, Map<String, ScriptEngineFactory> associations, Function<ScriptEngineFactory, List<String>> valuesFn) {
No objection to do some modernization of this code but probably best to avoid introducing overly long lines as it is impossible to see the changes with side-by-side diffs. ------------- PR: https://git.openjdk.java.net/jdk/pull/3229
On Sat, 27 Mar 2021 15:46:36 GMT, Alan Bateman <alanb@openjdk.org> wrote:
Attila Szegedi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 13 new commits since the last revision:
- Tidy - require non null in SimpleBindings - Simplify SimpleScriptContext.scopes - Mark fields final where possible - Deduplicate registerEngineXxx methods - Misc tidying - Deduplicate exception reporting - Lambdify - Mark fields as final; eliminate now unnecessary init() method. - Deduplicate engine creation and setup code - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f378f350...56d89eb2
src/java.scripting/share/classes/javax/script/ScriptEngineManager.java line 215:
213: } 214: 215: private ScriptEngine getEngineBy(String selector, Map<String, ScriptEngineFactory> associations, Function<ScriptEngineFactory, List<String>> valuesFn) {
No objection to do some modernization of this code but probably best to avoid introducing overly long lines as it is impossible to see the changes with side-by-side diffs.
Fair enough, I reformatted lines longer than 120 characters. ------------- PR: https://git.openjdk.java.net/jdk/pull/3229
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place of null checks followed by NPE throws, mark private fields final where possible, use lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where possible, and generally sanitize/deduplicate.
Attila Szegedi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 13 new commits since the last revision: - Tidy - require non null in SimpleBindings - Simplify SimpleScriptContext.scopes - Mark fields final where possible - Deduplicate registerEngineXxx methods - Misc tidying - Deduplicate exception reporting - Lambdify - Mark fields as final; eliminate now unnecessary init() method. - Deduplicate engine creation and setup code - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f378f350...56d89eb2 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/3229/files - new: https://git.openjdk.java.net/jdk/pull/3229/files/f378f350..56d89eb2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3229&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3229&range=00-01 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3229/head:pull/3229 PR: https://git.openjdk.java.net/jdk/pull/3229
On Sun, 28 Mar 2021 13:38:51 GMT, Attila Szegedi <attila@openjdk.org> wrote:
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place of null checks followed by NPE throws, mark private fields final where possible, use lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where possible, and generally sanitize/deduplicate.
Attila Szegedi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 13 new commits since the last revision:
- Tidy - require non null in SimpleBindings - Simplify SimpleScriptContext.scopes - Mark fields final where possible - Deduplicate registerEngineXxx methods - Misc tidying - Deduplicate exception reporting - Lambdify - Mark fields as final; eliminate now unnecessary init() method. - Deduplicate engine creation and setup code - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f378f350...56d89eb2
Marked as reviewed by sundar (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/3229
On Tue, 30 Mar 2021 12:43:18 GMT, Athijegannathan Sundararajan <sundar@openjdk.org> wrote:
Attila Szegedi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 13 new commits since the last revision:
- Tidy - require non null in SimpleBindings - Simplify SimpleScriptContext.scopes - Mark fields final where possible - Deduplicate registerEngineXxx methods - Misc tidying - Deduplicate exception reporting - Lambdify - Mark fields as final; eliminate now unnecessary init() method. - Deduplicate engine creation and setup code - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f378f350...56d89eb2
Marked as reviewed by sundar (Reviewer).
Thank you for the review @sundararajana! Much appreciated. ------------- PR: https://git.openjdk.java.net/jdk/pull/3229
On Sat, 27 Mar 2021 15:19:37 GMT, Attila Szegedi <attila@openjdk.org> wrote:
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place of null checks followed by NPE throws, mark private fields final where possible, use lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where possible, and generally sanitize/deduplicate.
This pull request has now been integrated. Changeset: 2bd80f94 Author: Attila Szegedi <attila@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/2bd80f94 Stats: 227 lines in 5 files changed: 40 ins; 140 del; 47 mod 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation Reviewed-by: sundar ------------- PR: https://git.openjdk.java.net/jdk/pull/3229
participants (3)
-
Alan Bateman
-
Athijegannathan Sundararajan
-
Attila Szegedi