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

make is_constant() recognize constants with several unary prefixes #1142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WofWca
Copy link
Contributor

@WofWca WofWca commented Feb 6, 2022

E.g. previously it would return false for expressions like void !0

E.g. previously it would return `false` for expressions like `void !0`
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.

LGTM, but can you add a test case?

@WofWca
Copy link
Contributor Author

WofWca commented Feb 7, 2022

Do you have an idea where to put it? Because this function looks rather internal. Best I can think of right now is adding something like { ...~!-+0 } -> {}, but then this change would need to be rebased onto #1141 (btw the evaluate option must be false).

@jridgewell
Copy link
Collaborator

You're right, after looking into the codebase there's not a great spot. The only one that I can see is in BinaryExpression, but it won't be a great test case regardless. This change could be merged into #1141 since that's the only observable case so far.

@WofWca
Copy link
Contributor Author

WofWca commented Feb 9, 2022

Maybe then it's better to just merge this and #1141 first, then add a test?

@fabiosantoscode
Copy link
Collaborator

Sounds like a good plan @WofWca

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