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

collapsing switch cases with same bodies, when not lined in a row #1070

Merged

Conversation

elad-yosifon
Copy link
Contributor

This is an additional optimization that leverages changes from #1044 @jridgewell

Current code does not fully optimize the following code:

 switch (_var) {
    case 1: return true;
    case 2: return true;
    case 3: return false;
    case 4: return true;
    case 5: return false;
    case 6: return false;
    case 7: return true;
    case 8: return true;
}

The current output returns something like this:

 switch (_var) {
    case 1: case 2: 
      return true;
    case 3: 
      return false;
    case 4:
      return true;
    case 5: case 6: 
      return false;
    case 7: case 8: 
      return true;
}

The expected output in this case should be:

switch (_var) {
    case 1: case 2: case 4: case 7: case 8: 
      return true;
    case 3: case 5: case 6: 
      return false;
}

Adding a default case can benefit the optimization even more:

 switch (_var) {
    case 1: return true;
    case 2: return true;
    case 3: return false;
    case 4: return true;
    case 5: return false;
    case 6: return false;
    case 7: return true;
    case 8: return true;
    default: return true;
}

The expected output here should be:

switch (_var) {
    default: 
      return true;
    case 3: case 5: case 6: 
      return false;
}

lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
test/compress/switch.js Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
test/compress/switch.js Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
lib/compress/index.js Outdated Show resolved Hide resolved
@fabiosantoscode
Copy link
Collaborator

Wondering if @jridgewell wants to give a final approval. It looks great to merge to me, especially those tests <3.

Copy link
Collaborator

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd already approved. 😉

@elad-yosifon
Copy link
Contributor Author

@fabiosantoscode @jridgewell great, what's next?

@jridgewell
Copy link
Collaborator

@fabiosantoscode will be able to merge. For the next PR, we could work on getting inert_branch performing better (the test case in #1070 (comment) isn't eliminating as well as it could), or work on reordering in between side-effect branch expressions (#1070 (comment)).

@fabiosantoscode
Copy link
Collaborator

Hey! This is indeed good to merge! Thanks @elad-yosifon 🚀

@fabiosantoscode fabiosantoscode merged commit 8430c66 into terser:master Sep 19, 2021
@elad-yosifon elad-yosifon deleted the collapse_same_branches_not_in_a_row branch September 19, 2021 22:13
jridgewell added a commit to jridgewell/terser that referenced this pull request Oct 10, 2021
@LongTengDao
Copy link
Contributor

@fabiosantoscode maybe a option to disable this would be better? because runtime order may be optimized, and will slow if do this

@elad-yosifon
Copy link
Contributor Author

@LongTengDao ATM this is buggy, so I think this should be disabled but not discarded.
I might be able to do some work on it this weekend..

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.

None yet

4 participants