-
Notifications
You must be signed in to change notification settings - Fork 14
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
🧊 Use forall_cof from Kado #362
Conversation
let forall sym = | ||
let i = Dim.DimProbe sym in | ||
extend @@ | ||
function |
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 am still a tiny bit concerned about the invariants here. But I do not have a counterexample... @favonia Can you perhaps explain why you are mostly certain this is OK?
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 believe I did not change the functionality of the code---that is, it is wrong iff it was wrong. Notice that there are two forall now---one is syntactic (cheap) and one is semantic (expensive), and it is the semantic (expensive) one used here. I am worried about the invariant but I was already worried before this PR. Does this answer your question?
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.
Where is the expensive one located now?
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.
The syntactic forall: https://redprl.org/kado/kado/Kado/Builder/Endo/module-type-S/index.html#val-forall
The semantic forall: https://redprl.org/kado/kado/Kado/Theory/Make/Disj/index.html#val-forall_cof
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.
Oh cool!! Sorry for my 🌰 🧠
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.
Where is the expensive one located now?
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.
Where is the expensive one located now?
Most relevant code (but I also changed other parts to make it work): https://github.com/RedPRL/kado/blob/b23ff87cfbaab6a5d35019f49da69227a252dcc6/src/Theory.ml#L392-L414
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.
Full 🚂 ahead!
See RedPRL/kado#6.