Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Complete model #1019
Complete model #1019
Changes from all commits
b29a119
959bf7b
8a556a4
3e2d501
6e499aa
02208a1
e07bfa0
b073017
ad415e5
14e729d
c65d8f2
359ac56
98899b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unless there is a reason to pass the
ref
around, it is usually better to usemutable
record fields rather thanref
s, both for performance (it is slightly more efficient, although as we have established it won't matter here) and to make the intent more clear, in particular that there will be no sharing of the field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my first attempt of course for sake of performance but it doesn't work. It seems we need to share this field between several environments so we cannot avoid the double indirection with the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that may sound like a bug? We do create new environments (copies) all the time, but the old ones should no longer be used (we overwrite the fields in
Fun_sat_frontend
). Thepop
issue should also no longer be relevant, because we can write to the mutable field here. Can you elaborate on what "it doesn't work" means?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check what's happened. When I tried using a mutable field instead of a reference, FunSAT keeps declarations of popped assertion levels. Then I switched to a reference (I only did this modification) and it magically worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that with a mutable field you need to set
acc.declare_top <- Stack.pop acc.declare_tail
notenv.declare_top <- Stack.pop env.declare_tail
in thepop
function (which I'd guess is the issue given what you describe).