Remove unused fn params #182
Conversation
|
The tests fail right now because I've set the default as to remove unused fn params and haven't modified other tests. Should it be opt-in instead of opt-out ? |
|
I think |
|
Removing them by default seems fine if we're doing same with names. IMO, if we start getting complains we can change. |
|
Cool - Hit a bug - |
|
Updated. |
|
Great. Could you update benchmarks table as well. |
|
The diff is -uglify 21.79kB 221% 7.29kB 169% 1ms 247ms
-closure 21.67kB 223% 7.37kB 167% 2ms 1223ms
-babili 21.9kB 219% 7.46kB 164% 2ms 722ms
-closure js 24.01kB 191% 8.04kB 144% 2ms 3301ms
+uglify 21.79kB 221% 7.29kB 169% 2ms 387ms
+closure 21.67kB 223% 7.37kB 167% 2ms 3195ms
+babili 21.86kB 220% 7.46kB 163% 2ms 943ms
+closure js 24.01kB 191% 8.04kB 144% 2ms 4943mswe need to move running benchmarks on same machine / with same hardware config. maybe on CI? |
| // this is used for tracking fn params that can be removed | ||
| // as traversal takes place from left and | ||
| // unused params can be removed only on the right | ||
| const markForRemoval = Symbol("markForRemoval"); |
There was a problem hiding this comment.
Nit: shouldn't it be markedForRemoval?
There was a problem hiding this comment.
hmmm. IMO, depends on context,
at the place of assignment, mark this for removal,
at the place of verifying, is this marked for removal,
Not sure. IMO, either way should be fine.
| break; | ||
| } | ||
|
|
||
| Object.defineProperty(binding, markForRemoval, { |
There was a problem hiding this comment.
Do we need to defineProperty here instead of just assigning it? Is this because we don't want to taint binding with "irrelevant" enumerable prop? I'm just worried about performance.
There was a problem hiding this comment.
Ohhh . defineProp has perf drawbacks!! didn't know. I just didn't want it to be enumerable.
|
We should update the readme when possible when changing options + explanation |
keepFnArgskeepFnamestokeepFnNamefor DCE