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

The formulae top and bot are constant #868

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

Halbaroth
Copy link
Collaborator

This PR makes clear that the environments of both CC(X) and UF(X) are constant. The previous signature of the constructors for these environments was unit -> t and not t only because they use the top and bot semantic values. As the module Combine in Shostak cannot contain constants because of limitations with recursive module in OCaml, we have put these constants in a trivial closure.

I create a new module X which is an alias of Shostak.Combine and contains the constants top and bot.

Note: I don't remove all the aliases module X = Shostak.Combine in many modules of AE. We can do it gradually and modifying all the files will be painful while rebasing our current PRs.

Note: I cannot turn the environment of Theory into a constant because this environment uses mutable states to enqueue facts. I plan to fix that in another PR (probably by writing a module for functional queues but I have to check how the current environment behaves while backtracking).

@bclement-ocp
Copy link
Collaborator

bclement-ocp commented Oct 10, 2023

This is somewhat conflicting with the direction I thought were trying to take with #798. Can you explain why it is important that these environments are constants?

I would also advise against writing functional queues for the Theory module. As far as I know, the queues in Theory are used locally and are empty between steps, we would never make use of the extra functionality provided by a functional queue compared to an imperative one.

@Halbaroth
Copy link
Collaborator Author

Halbaroth commented Oct 10, 2023

I think the main change here is to make clear that top and bot are constant. I can change the title. The reason why I did that it's because I want to understand why the environments of these modules seem to be imperative. If there is no reason to be imperative, we should make it clear in the signature. You can modify the signature again when you will merge your PR with a new UF.

@bclement-ocp
Copy link
Collaborator

I have given a longer thought about this, and I really don't like the introduction of X as a toplevel module (especially when it is a common alias in the rest of the code, and even further when it is mostly empty). Can we instead add the definitions:

let top = top ()
let bot = bot ()

directly in Shostak.Combine (but not CX), with the appropriate signature change?

@bclement-ocp
Copy link
Collaborator

bclement-ocp commented Oct 23, 2023 via email

The `top` and `bottom` are constant but we use closures to define them
because of limitation of the recursive module. This commit refactors the
way we define them. Now, `top` and `bot` are constants exposed by the
module `Shostak.Combine`.
@Halbaroth
Copy link
Collaborator Author

You should be able to do module Combine : sig include Sig.X val top : r val bot : r end shadowing the unit -> r versions of top and bot I think.

I removed my comment because I found this solution... :D

Copy link
Collaborator

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

Ah, I was not sure if this was a GitHub bug that it didn't appear in the UI hence why I replied by email :) All good then!

@Halbaroth Halbaroth changed the title The environments of CC(X) and UF(X) are constant The formulae top and bot are constant Oct 23, 2023
@Halbaroth Halbaroth enabled auto-merge (squash) October 23, 2023 08:54
@Halbaroth Halbaroth merged commit a5e1347 into OCamlPro:next Oct 23, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants