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

[Lang] Add option short_circuit_operators for short-circuiting boolean ops #3632

Merged
merged 1 commit into from
Nov 28, 2021

Conversation

re-xyr
Copy link
Contributor

@re-xyr re-xyr commented Nov 27, 2021

Related issue = Fixes #3572

cc @strongoier

ti.init(short_circuit_operators=True)

@CLAassistant
Copy link

CLAassistant commented Nov 27, 2021

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Nov 27, 2021

✔️ Deploy Preview for jovial-fermat-aa59dc canceled.

🔨 Explore the source changes: ecc5e92

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/61a2d7bc510b850007dd6267

Copy link
Contributor

@strongoier strongoier left a comment

Choose a reason for hiding this comment

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

Great work! Only a tiny thing to notice below.

tests/python/test_short_circuit.py Show resolved Hide resolved
Copy link
Contributor

@lin-hitonami lin-hitonami left a comment

Choose a reason for hiding this comment

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

IIUC, this method always needs len(node.values) - 1 times of comparison when executing. When the program meets a false in and operation, the comparison after it is not necessary. This can be achieved by generating nested ifs.

@strongoier
Copy link
Contributor

strongoier commented Nov 27, 2021

IIUC, this method always needs len(node.values) - 1 times of comparison when executing. When the program meets a false in and operation, the comparison after it is not necessary. This can be achieved by generating nested ifs.

I agree. Your comment reminds me that Python AST treats BoolOp differently from BinaryOp, which naturally fits your suggestion.

@re-xyr FYI https://docs.python.org/3/library/ast.html#ast.BoolOp.

@re-xyr
Copy link
Contributor Author

re-xyr commented Nov 27, 2021

this method always needs len(node.values) - 1 times of comparison when executing

nice catch - will fix this. Not sure why Python doesn't just make boolean operators right-associative though.

Copy link
Contributor

@lin-hitonami lin-hitonami left a comment

Choose a reason for hiding this comment

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

Although the expected behavior of bool expressions is to be determined, other parts look good to me. Thanks!

Copy link
Contributor

@strongoier strongoier left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@strongoier strongoier changed the title [Lang] Add support for short-circuiting boolean operators [Lang] Add option short_circuit_operators for short-circuiting boolean ops Nov 28, 2021
@strongoier strongoier changed the title [Lang] Add option short_circuit_operators for short-circuiting boolean ops [Lang] Add option short_circuit_operators for short-circuiting boolean ops Nov 28, 2021
@strongoier strongoier merged commit fdf665e into taichi-dev:master Nov 28, 2021
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.

Support short-circuiting logical expressions
4 participants