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

More boolean simplifications in generated code. #7149

Merged
merged 6 commits into from
Nov 8, 2024

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Nov 7, 2024

This handles all the boolean simplifications in #7142.
What's left from that issue is conditionals b ? x : y, that can be done separately.

@cristianoc cristianoc requested review from cknitt and zth November 8, 2024 09:11
Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

Really nice! 👏

@cristianoc cristianoc enabled auto-merge (rebase) November 8, 2024 10:15
@cristianoc cristianoc merged commit 7a58f3b into master Nov 8, 2024
19 checks passed
let res =
match (e1.expression_desc, e2.expression_desc) with
| Bool false, _ -> Some false_
| _, Bool false -> Some false_
Copy link

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! It does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

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? 👀

Copy link

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
    match (a.expression_desc, b.expression_desc) with
    | ( (Null | Undefined _),
       (Bool _ | Number _ | Typeof _ | Fun _ | Array _ | Caml_block _) ) ->
    the Typeof or Array or Caml_block could have side effects, so can't always be safely dropped
  • in int_comp
      | Ceq, Optional_block _, Undefined _ | Ceq, Undefined _, Optional_block _ ->
     false_
    if Optional_block can have side effects then it shouldn't be dropped
  • in 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.

Copy link
Collaborator Author

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

    match (a.expression_desc, b.expression_desc) with
    | ( (Null | Undefined _),
       (Bool _ | Number _ | Typeof _ | Fun _ | Array _ | Caml_block _) ) ->

    the Typeof or Array or Caml_block could have side effects, so can't always be safely dropped

  • in int_comp

      | Ceq, Optional_block _, Undefined _ | Ceq, Undefined _, Optional_block _ ->
     false_

    if Optional_block can have side effects then it shouldn't be dropped

  • in 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.

Thanks a lot @yallop !
I've fixed a bunch more in #7151

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.

3 participants