Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix "use asm" numeric output in function expressions #2328

Merged
merged 1 commit into from
Sep 20, 2017
Merged

fix "use asm" numeric output in function expressions #2328

merged 1 commit into from
Sep 20, 2017

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Sep 20, 2017

Fixes: #2324

@alexlamsl
Copy link
Collaborator

Thanks for beating me to it 😉

@alexlamsl alexlamsl merged commit ccd1bdf into mishoo:master Sep 20, 2017
@kzc
Copy link
Contributor Author

kzc commented Sep 20, 2017

@alexlamsl Glad to help out.

I've spotted an inefficiency - if self is an AST_Scope then it can't be an AST_Directive:

--- a/lib/output.js
+++ b/lib/output.js
@@ -491,7 +491,7 @@ function OutputStream(options) {
         if (self instanceof AST_Scope) {
             active_scope = self;
         }
-        if (!use_asm && self instanceof AST_Directive && self.value == "use asm") {
+        else if (!use_asm && self instanceof AST_Directive && self.value == "use asm") {
             use_asm = active_scope;
         }
         function doit() {

Would you mind committing that change?

alexlamsl pushed a commit that referenced this pull request Sep 20, 2017
alexlamsl pushed a commit that referenced this pull request Sep 20, 2017
@alexlamsl
Copy link
Collaborator

Would you mind committing that change?

Done in 7e3e9da

@kzc
Copy link
Contributor Author

kzc commented Sep 20, 2017

Thanks.

Just curious - my git-fu is weak - did you do a git rebase on master? I thought that was against the law!

$ git fetch upstream
remote: Counting objects: 4, done.
remote: Total 4 (delta 3), reused 4 (delta 3), pack-reused 0
Unpacking objects: 100% (4/4), done.
From https://github.com/mishoo/UglifyJS2
 + ccd1bdf...7e3e9da master     -> upstream/master  (forced update)

$ git merge upstream/master
Auto-merging lib/output.js
CONFLICT (content): Merge conflict in lib/output.js
Automatic merge failed; fix conflicts and then commit the result.

The conflict was the line in question.

My local repo fix was to git reset 00f5094

git reset 00f509405b44882bab5d63d97caacb21691cdad6
git checkout .
git merge upstream/master

@alexlamsl
Copy link
Collaborator

alexlamsl commented Sep 20, 2017

I applied your patch, then git commit --amend, followed by git push -f

On my local branch I did git reset --hard HEAD~1 then git pull which saves me from working out the new commit's hash.

(git rebase would have altered all the commit hashes, and that would be criminal 😅)

@kzc
Copy link
Contributor Author

kzc commented Sep 20, 2017

Okay, it's cool. Just the one or two people who updated their local master in the intermediate hour between the last two commits would have been affected. No sweat.

thasso added a commit to castlabs/UglifyJS2 that referenced this pull request Aug 1, 2018
@kzc kzc deleted the use_asm_output_scope branch October 30, 2018 22:31
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