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

Update saw-core submodule to fix addsimp crash. #965

Merged
merged 1 commit into from
Dec 10, 2020
Merged

Conversation

brianhuffman
Copy link
Contributor

This patch includes PR GaloisInc/saw-core#112, which
implements error handling using the Maybe monad for
the creation of rewrite rules from equational theorems.

Fixes #319.

Copy link
Contributor

@atomb atomb left a comment

Choose a reason for hiding this comment

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

This is great. I had one comment about error handling, but don't have really strong feelings one way or the other.

addsimp :: Theorem -> Simpset -> TopLevel Simpset
addsimp (Theorem (Prop t) _stats) ss =
case ruleOfProp t of
Nothing -> fail "addsimp: theorem not an equation"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely settled on the right answer here, but I've been using throwTopLevel in some code recently when working in the TopLevel monad. It doesn't hugely matter, because fail calls throwTopLevel in this case, but I do slightly like the idea of avoiding having calls to fail in the codebase, even if we know they're well-behaved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree that fail is not the way to go in the long term. However there are already lots of other TopLevel primitives that use fail in their implementations right now, so I just used fail for consistency. I was thinking that we could remove all the uses of fail in the TopLevel monad in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that makes sense. The same question will come up shortly when I merge the external ABC interaction branch. I did modify some code to use throwTopLevel, but there's a bunch more refactoring to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just opened #966, which I imagine would be reasonable to address in a single PR.

This patch includes PR GaloisInc/saw-core#112, which
implements error handling using the Maybe monad for
the creation of rewrite rules from equational theorems.

Fixes #319.
@brianhuffman brianhuffman merged commit 8385cb4 into master Dec 10, 2020
@brianhuffman brianhuffman deleted the issue319 branch January 26, 2021 03:58
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.

addsimp could do more input checking; printing should not exit the REPL
2 participants