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

[WIP] LLVM term annotations #453

Merged
merged 10 commits into from
Apr 2, 2020
Merged

[WIP] LLVM term annotations #453

merged 10 commits into from
Apr 2, 2020

Conversation

robdockins
Copy link
Contributor

Another, more cleaned-up take on addressing #425

robdockins and others added 8 commits March 26, 2020 13:57
These are: incomplete-patterns, overlapping-patterns, and missing-methods.
Remove the use of the `WithAssertions` expression former in crucible-llvm.
Instead, a language-extension expression former `LLVM_SideConditions` has
been added that directly references the `LLVM_SafetyCondition` type.

Remove the `WithAssertions` syntax constructor and related
typeclass constraints.
mechanism, and the `HasStructuredAssertions` typeclass.

These ideas are largely being replace with term annotations.
Copy link
Contributor

@andreistefanescu andreistefanescu left a comment

Choose a reason for hiding this comment

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

Looks good!

@robdockins
Copy link
Contributor Author

I'm not convinced about 0622138 at all; panic should only be used for situations where the situation should never happen, regardless of user input. That really doesn't seem to be the case here.

@andreistefanescu
Copy link
Contributor

muxLLVMVal is used on two LLVMVals that are obtained by reading from the memory the same MemType on the two branches of a WriteMerge. I don't think that panic can happen.

@robdockins
Copy link
Contributor Author

Ah, I see. Yeah, tracing it back, it looks like all call sites come from places where it is plausible that the values should be structurally compatible. I'll add a comment to that effect.

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.

None yet

2 participants