Do not produce let as a variable name in mangle.
#1011
Conversation
Would previously occur in large generated functions with 21,000+ variables. Fixes #986.
|
|
||
| // Produce a lot of variables in a function and run it through mangle. | ||
| var s = '"use strict"; function foo() {'; | ||
| for (var i = 0; i < 21000; ++i) { |
There was a problem hiding this comment.
Why only 21e3? Should be Math.pow(64, 3) + Math.pow(64, 2) + 64 or 266304.
There was a problem hiding this comment.
Because it takes too long to test. On my machine the mangle test for 21,000 variables is 1 second, but on Travis CI machine it is over 2 seconds. 266K vars would take around 24s or so on Travis CI.
There was a problem hiding this comment.
The test looks for the symbols that had existed before and after the erroneously generated let. If the mangle algorithm changes, this test would have to be updated.
But if @mishoo or @rvanvelzen would be good with a 50 second npm test (2X estimated Travis CI run time) to test all 3 letter mangle var names, then we could do that.
Edit: node 5.8.0 cannot handle running an uglifyjs mangle against a function with more than 126,000 variables - Maximum call stack size exceeded. So that's a no go.
|
@mishoo or @rvanvelzen Could you please commit this obvious one line bug fix? |
|
I'd be OK with no test here. I'm not the guy to believe that every fix should be accompanied by a test case. Some bugs, once fixed, will stay fixed — there's no reason why someone would later remove Any case, it runs in like one second here, it's acceptable. Definitely let's not increase that. |
|
@mishoo - I agree this test is over the top. I just wanted to prove to myself that it worked in a non-API-changing intrusive manner. I'll submit a PR to remove the test. |
|
@kzc no no, just leave it, it's fine. Just wanted to say that as far as I am concerned, you need not stress too much about tests. |
Fixes #986 based on patch originally provided by @pablocubico.
Bug would previously occur in large generated functions with 21,000+ variables.
May have also happened with emscripten asm.js output.