Skip to content

matching: minor simplification in do_for_multiple_match#13082

Merged
gasche merged 1 commit into
ocaml:trunkfrom
gasche:matching-clarify-do_for_multiple_match
Apr 8, 2024
Merged

matching: minor simplification in do_for_multiple_match#13082
gasche merged 1 commit into
ocaml:trunkfrom
gasche:matching-clarify-do_for_multiple_match

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Apr 8, 2024

(This is a small refactoring commit that should be obviously correct.)

do_for_multiple_match is used to optimize code of the form match e1, e2 with ..., to avoid allocating a tuple (e1, e2) when possible (when the pattern does not require binding the scrutinee). (If the clause is | (x, y) as p -> ... for example, a binding let p = (e1, e2) in ... will still be emitted.)

At first sight, its implementation is wrong. It precompiles a set of pattern-matching clauses whose scrutinee is a tuple of arbitrary expressions, and then it binds each of those expressions to a fresh variable and transforms the result of the precompilation to use these per-element variables instead of the tuple, to save an allocation. This is wrong because the arbitrary expressions may thus occur twice in the generated code, once in a let p = (e1, e2) binding generated by the precompilation, and another to bind the per-element variables.

In fact this is okay because there is only one caller for do_for_multiple_match, and it always passes a tuple of expressions that are in fact variables, by performing an "outer binding" step that binds all the tuple elements to and calls do_for_multiple_match on a list of expressions-that-are-variables. So in fact there is no bug, and the fresh per-element variables in do_for_multiple_match are never useful.

This redundant binding of variables was introduced in 2009 in f8107f9, to fix the bug described above (duplication of effectful expressions in the generated code), see #4828.

The present commit simplifies and clarifies the code by changing do_for_multiple_match to take a list of variables instead of a list of expressions, and removing the redundant creation of per-element variables.

(cc @ncik-roberts, @trefis, @Octachron, @lthls)

(This is a small refactoring commit that should be obviously correct.)

`do_for_multiple_match` is used to optimize code of the form
`match e1, e2 with ...`, to avoid allocating a tuple `(e1, e2)` when
possible (when the pattern does not require binding the
scrutinee). (If the clause is `| (x, y) as p -> ...` for example, a
binding `let p = (e1, e2) in ...` will still be emitted.)

At first sight, its implementation is wrong. It precompiles a set of
pattern-matching clauses whose scrutinee is a tuple of arbitrary
expressions, and then it binds each of those expressions to a fresh
variable and transforms the result of the precompilation to use these
per-element variables instead of the tuple, to save an
allocation. This is wrong because the arbitrary expressions may thus
occur twice in the generated code, once in a `let p = (e1, e2)`
binding generated by the precompilation, and another to bind the
per-element variables.

In fact this is okay because there is only one caller for
`do_for_multiple_match`, and it always passes a tuple of expressions
that are in fact variable, by performing an "outer binding" step that
binds all the tuple elements to and calls `do_for_multiple_match` on a
tuple of variables. So in fact there is no bug, and the fresh
per-element variables in `do_for_multiple_match` are never useful.

This redundant binding of variables was introduced in 2009 in
f8107f9, to fix the bug described
above (duplication of effectful expressions in the generated code),
see ocaml#4828.

The present commit simplifies and clarifies the code by changing
`do_for_multiple_match` to take a list of variables instead of a list
of expressions, and removing the redundant creation of per-element
variables.
Copy link
Copy Markdown
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

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

This code looked familiar, and apparently I already spotted that while working on #12440. Thanks for actually taking care of it.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Apr 8, 2024

Thanks both for the prompt reviews, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants