-
Notifications
You must be signed in to change notification settings - Fork 471
More boolean simplifications in generated code. #7149
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice! 👏
| let res = | ||
| match (e1.expression_desc, e2.expression_desc) with | ||
| | Bool false, _ -> Some false_ | ||
| | _, Bool false -> Some false_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a check that e1 doesn't have side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yallop nice find! See any more issues of that kind? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there seem to be a few. To pick some at random:
- in
eq_null_undefined_booleanthematch (a.expression_desc, b.expression_desc) with | ( (Null | Undefined _), (Bool _ | Number _ | Typeof _ | Fun _ | Array _ | Caml_block _) ) ->
TypeoforArrayorCaml_blockcould have side effects, so can't always be safely dropped - in
int_compif| Ceq, Optional_block _, Undefined _ | Ceq, Undefined _, Optional_block _ -> false_
Optional_blockcan have side effects then it shouldn't be dropped - in
simplify_orthe left operand shouldn't be dropped if it has side effects.| _, Bool true -> Some true_
This is just a selection; there are some similar cases in those functions and others. I don't have all the development tools set up, so can't easily send a PR, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there seem to be a few. To pick some at random:
match (a.expression_desc, b.expression_desc) with | ( (Null | Undefined _), (Bool _ | Number _ | Typeof _ | Fun _ | Array _ | Caml_block _) ) ->the
TypeoforArrayorCaml_blockcould have side effects, so can't always be safely droppedin
int_comp| Ceq, Optional_block _, Undefined _ | Ceq, Undefined _, Optional_block _ -> false_if
Optional_blockcan have side effects then it shouldn't be droppedin
simplify_or| _, Bool true -> Some true_the left operand shouldn't be dropped if it has side effects.
This is just a selection; there are some similar cases in those functions and others. I don't have all the development tools set up, so can't easily send a PR, unfortunately.
This handles all the boolean simplifications in #7142.
What's left from that issue is conditionals
b ? x : y, that can be done separately.