Skip to content

CAD-4750 generalise OrElse frame #15

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

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

yogeshsajanikar
Copy link
Contributor

OrElse and Catch have similar control semantics. Both have alternate branches that needs to be considered when a certain condition is encountered (retry in OrElse, throw in Catch). This commit tries to generalise OrElseLeftFrame/OrElseRightFrame to handle these control semantics in a similar fashion.

The commit introduces BranchFrame that indicates that a branch has an alternate execution that is indicated by either a "statement" (OrElseStmA) or an empty statement (to indicate that there is no alternate execution left, this replaces OrElseRightFrame).

This should allow adding Catch to IO-Sim a relatively easily, than introducing a new frame to handle Catch separately.

@yogeshsajanikar
Copy link
Contributor Author

This pull request https://github.com/input-output-hk/io-sim/pull/13 that implements catch frame separately.

@yogeshsajanikar
Copy link
Contributor Author

This #15 does not introduce a new semantics, but just simplifies OrElseLeftFrame and replaces it by BranchFrame. The PR indicates that BranchFrame works the same for OrElse without introducing any regressions. The next PR #16 includes this change and adds catch support on top of this.

Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

Nice simplification! Only a few minor comments.

Note that I did not comment on the IOSimPOR part it is identical to IOSim in this case (which makes sense as this code is not scheduling-related).

Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

LGTM

@yogeshsajanikar yogeshsajanikar marked this pull request as ready for review August 3, 2022 13:52
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Approved. I offered a couple minor/optional suggestions.

Though I suggest getting @coot 's review before merging.

@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4750-generalise-orelse-frame branch 3 times, most recently from 9683782 to 6bf6856 Compare August 7, 2022 14:13
@yogeshsajanikar
Copy link
Contributor Author

@coot I ran the tests, and they passed for ouroboros-network with this commit (6bf6856)

cabal run ouroboros-network-framework:test` -> All 47 tests passed (114.99s)
cabal run ouroboros-network:test -> All 314 tests passed (609.21s)

Copy link
Collaborator

@coot coot left a comment

Choose a reason for hiding this comment

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

Very nice, thanks! I'd only would make BranchStmA a strict field (as suggested by @nfrisby): since we are providing another level of abstraction / indirection, making this field strict will eliminate the thunk before IOSim would evaluate it.

@yogeshsajanikar
Copy link
Contributor Author

resolves IntersectMBO/ouroboros-network#1461

@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4750-generalise-orelse-frame branch 2 times, most recently from e38dcf0 to d329b00 Compare August 18, 2022 15:21
@yogeshsajanikar yogeshsajanikar requested a review from coot August 18, 2022 15:35
@yogeshsajanikar
Copy link
Contributor Author

@coot I did the minor changes that you suggested. And also deduplicated few lines as @nfrisby suggested.

Copy link
Collaborator

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

OrElseLeftFrame and OrElseRightFrame represent a control structure that
has a alternative branch that is executed when `retry` is applied. A
`catch` has a similar execution model when a `throw` is applied.

The control frame is generalised to BranchFrame that can hold an
alternative statement. If the execution context is `left` side of the
branch then the BranchFrame contains `right` statement. When we are
executing in the `right` context, the branch frame contains an empty
statement.
@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4750-generalise-orelse-frame branch from d555c48 to ccc84f8 Compare August 24, 2022 05:47
@yogeshsajanikar yogeshsajanikar merged commit c720399 into main Aug 24, 2022
@yogeshsajanikar yogeshsajanikar deleted the CAD-4750-generalise-orelse-frame branch August 24, 2022 05:57
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.

4 participants