Consolidate multiple optimization levels down to one #1754
Conversation
Remove them from main code and replace with "interpreted mode" flag. Disable deprecation warnings in tests because we still want to test the old stuff for now at least.
| @@ -80,8 +80,7 @@ public String[] processOptions(String args[]) { | |||
| continue; | |||
| } | |||
| if ((arg.equals("-opt") || arg.equals("-O")) && ++i < args.length) { | |||
| int optLevel = Integer.parseInt(args[i]); | |||
| compilerEnv.setOptimizationLevel(optLevel); | |||
| // This no longer has an effect, but parse it for backward compatibility | |||
There was a problem hiding this comment.
maybe we can add a version hint to the comment, i fear when reading this later someone likes to know the version that introduces this change
There was a problem hiding this comment.
Check out new comments, thanks!
rhino-engine/src/main/java/org/mozilla/javascript/engine/RhinoScriptEngine.java
Outdated
Show resolved
Hide resolved
rhino-engine/src/main/java/org/mozilla/javascript/engine/RhinoScriptEngine.java
Outdated
Show resolved
Hide resolved
| @@ -551,7 +551,7 @@ static void processFileSecure(Context cx, Scriptable scope, String path, Object | |||
| Object source = readFileOrUrl(path, !isClass); | |||
|
|
|||
| byte[] digest = getDigest(source); | |||
| String key = path + "_" + cx.getOptimizationLevel(); | |||
| String key = path + "_" + (cx.isInterpretedMode() ? "int" : "comp"); | |||
There was a problem hiding this comment.
'int' might be confusing, is there any good reason for this shortening?
There was a problem hiding this comment.
Not really -- AFAIK this is just used as a cache key but there's no reason why not to make it longer. (Previously it was just an integer.)
There was a problem hiding this comment.
Checkout the latest patch, it supports both "-int" and "-interpreted"
| @@ -34,7 +34,7 @@ msg.shell.usage =\ | |||
| \ -w Enable warnings.\n\ | |||
| \ -version 100|110|120|130|140|150|160|170|180|200\n\ | |||
| \ Set a specific language version.\n\ | |||
| \ -opt [-1|0-9] Set optimization level.\n\ | |||
| \ -int Run in interpreted mode.\n\ | |||
There was a problem hiding this comment.
Yes, although don't you think that '-interpreted' is too long? And '-I' is too much like the "-I" argument in a C compiler.
What if we support both "-int" and "-interpreted" command-line flags?
There was a problem hiding this comment.
although don't you think that '-interpreted' is too long?
sometimes, but i learned the hard way that is easier for me to type this many chars while writing the code & doc the spending the the answering all the questions ;-D
|
|
||
| return null; | ||
| }); | ||
| } |
There was a problem hiding this comment.
This function, and "getTestOptLevels", is no longer used. There's also a duplicate-ish version of this class in the "tests" module where this might be used more.
|
Another great step on our journey to modernize this |
* Add proper deprecation warnings * Rename old "doctests" files to make them clearer
|
@gbrail i just notice that we have the Utils class more or less twice
I think we have to merge it into one - but i'm to far away from gradle modules to understand how we can make the one from the rhino module available in the test module also. Maybe you can have a look at this (please) |
|
I'd like to address the "Utils" class in a different PR, but we can try some things. However, I think that we might need a new "test-utils" module to contain that if we want to do it the cleanest possible way. |
Instead of 10+ optimization, levels, reduce them to one -- "interpreted mode" and compiled mode. This includes: