Skip to content

smarter const replacement taking name length into account#1459

Closed
kzc wants to merge 1 commit into
mishoo:masterfrom
kzc:smarter_const_replace
Closed

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

Conversation

@kzc

@kzc kzc commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

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

kzc commented Feb 11, 2017

Copy link
Copy Markdown
Contributor Author

I changed this PR to deem AST_RegExp once again side-effect-free in has_side_effects() as per [email protected] to fix a regression. New test unused_regexp_literal added.

@kzc

kzc commented Feb 11, 2017

Copy link
Copy Markdown
Contributor Author

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

kzc commented Feb 23, 2017

Copy link
Copy Markdown
Contributor Author

@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