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

Fix roots in solution2evd #614

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tragicus
Copy link
Contributor

@Tragicus Tragicus commented Mar 19, 2024

It may happen that coq-elpi aliases something in the input evar_map and then drops the alias because it does not appear in the solution returned by coq-elpi. However, this alias might be referenced in the evar_map, so it should not be dropped. This PR fixes the issue by adding the evars from the input evar_map to the roots of the computation of reachable evars.
This relies on some API from coq that does not exist yet.

@CohenCyril
Copy link
Collaborator

@Tragicus could you link to a branch of Coq which exposes the primitive you needed?

@Tragicus
Copy link
Contributor Author

Yes, here it is (I only need the contents of engine/evd.ml): https://github.com/Tragicus/coq/tree/utils
For reference, I opened a PR for this branch: coq/coq#18820

@gares
Copy link
Contributor

gares commented Mar 20, 2024

I think the old code was actually implementing Evd.undefined_evars, about the other one I don't know.

@gares
Copy link
Contributor

gares commented Jun 28, 2024

@Tragicus can you tell me if this code does what you wanted?

Then we definitely need to do some benchmarking

@Tragicus
Copy link
Contributor Author

It was long ago so I might be mistaken, but I am pretty sure this solved our issue back then. I can do the benchmarking tomorrow.

@Tragicus
Copy link
Contributor Author

I can compile neither master nor coq-master neither on coq's master nor v8.19. Can you tell me which branches I should take?

@gares
Copy link
Contributor

gares commented Jun 30, 2024

Maybe you need to rebade this pr, but currently master works with 8.19, 8.20rc1 and master, both with opam and Nix.

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.

3 participants