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

Ensure switch branches reordering happens only for constant branch expressions #1092

Conversation

elad-yosifon
Copy link
Contributor

@fabiosantoscode
Copy link
Collaborator

Hey, this looks good to me!

Is this just for @jridgewell to merge in his PR, or is this the correct fix?

@elad-yosifon
Copy link
Contributor Author

@fabiosantoscode this is a “counter commit” to @jridgewell revert attempt. The existing code is buggy, this is a fix.

Comment on lines +2606 to +2614
case 3 == i:
console.log(i);
break;
case 5 == i:
console.log(5);
break;
case 1 == i:
console.log(i);
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'm not sure this properly catches the bug (but the output isn't optimized, so it's not a hard blocker). We need to test that non-mutually-exclusive branches are correct:

Suggested change
case 3 == i:
console.log(i);
break;
case 5 == i:
console.log(5);
break;
case 1 == i:
console.log(i);
break;
case 3 == i:
console.log('equals');
break;
case 5 > i:
console.log('greater than');
break;
case 1 == i:
console.log('equals');
break;

This case should log 'greater than', but if we did the optimization, it would log equals (because the 1 == i would have been moved up).

@jridgewell
Copy link
Collaborator

Let's merge this, and I'll work it into my PR.

@fabiosantoscode fabiosantoscode merged commit f251cd4 into terser:master Oct 19, 2021
@elad-yosifon elad-yosifon deleted the ensure_case_reordering_only_if_exp_is_constant branch October 23, 2021 16:52
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

3 participants