-
Notifications
You must be signed in to change notification settings - Fork 455
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
More boolean simplifications in generated code. #7149
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_boolean
thematch (a.expression_desc, b.expression_desc) with | ( (Null | Undefined _), (Bool _ | Number _ | Typeof _ | Fun _ | Array _ | Caml_block _) ) ->
Typeof
orArray
orCaml_block
could have side effects, so can't always be safely dropped - in
int_comp
if| Ceq, Optional_block _, Undefined _ | Ceq, Undefined _, Optional_block _ -> false_
Optional_block
can have side effects then it shouldn't be dropped - in
simplify_or
the 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
Typeof
orArray
orCaml_block
could have side effects, so can't always be safely droppedin
int_comp
| Ceq, Optional_block _, Undefined _ | Ceq, Undefined _, Optional_block _ -> false_if
Optional_block
can 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.