Skip to content

smarter const replacement taking name length into account#1459

Closed
kzc wants to merge 1 commit intomishoo:masterfrom
kzc:smarter_const_replace
Closed

smarter const replacement taking name length into account#1459
kzc wants to merge 1 commit intomishoo:masterfrom
kzc:smarter_const_replace

Conversation

@kzc
Copy link
Copy Markdown
Contributor

@kzc kzc commented Feb 2, 2017

Fixes: #1191, #1194, #1396

@alexlamsl
Copy link
Copy Markdown
Collaborator

Interesting maths. Changes don't conflict from any existing PRs queued for 2.7.5 as well, I think.

LGTM

@kzc kzc mentioned this pull request Feb 11, 2017
@kzc
Copy link
Copy Markdown
Contributor Author

kzc commented Feb 11, 2017

I changed this PR to deem AST_RegExp once again side-effect-free in has_side_effects() as per uglify-js@2.7.5 to fix a regression. New test unused_regexp_literal added.

@kzc
Copy link
Copy Markdown
Contributor Author

kzc commented Feb 11, 2017

is_constant() will still return false for AST_RegExp.

Perhaps is_immutable() would have been a better name for that method. I'll leave it for another day and another PR.

Edit: spelling fixed for is_constant().

alexlamsl pushed a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017
@alexlamsl
Copy link
Copy Markdown
Collaborator

Now that I've merged #1498, I see we can extend the logic here to include variables as well. Would that be undesirable?

@alexlamsl
Copy link
Copy Markdown
Collaborator

On second thought, let's worry about that in another PR after 2.8.0

@kzc
Copy link
Copy Markdown
Contributor Author

kzc commented Feb 23, 2017

@alexlamsl That would be a good addition post 2.8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants