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

Remove tactic hresolve_core #17035

Merged
merged 3 commits into from
Jan 4, 2023
Merged

Remove tactic hresolve_core #17035

merged 3 commits into from
Jan 4, 2023

Conversation

SkySkimmer
Copy link
Contributor

@SkySkimmer SkySkimmer commented Dec 29, 2022

The implementation is fragile due to being based on detyping and a loc hack, and it seems unused.

It was also not in the doc until
#16498 (8.17) and then only got a doc stub.

Overlays:

@SkySkimmer SkySkimmer requested review from a team as code owners December 29, 2022 13:10
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Dec 29, 2022
@SkySkimmer SkySkimmer added the needs: changelog entry This should be documented in doc/changelog. label Dec 29, 2022
SkySkimmer added a commit to SkySkimmer/paco that referenced this pull request Dec 29, 2022
@SkySkimmer
Copy link
Contributor Author

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Dec 29, 2022
@JasonGross
Copy link
Member

According to #16846 it is used to implement hpattern which looks like a useful tactic that I'd be interested in seeing upstreamed into Coq rather than broken

@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Dec 29, 2022
@SkySkimmer
Copy link
Contributor Author

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Dec 29, 2022
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Dec 29, 2022

🔴 CI failure at commit fd455be without any failure in the test-suite

✔️ Corresponding job for the base commit 41e76f5 succeeded

❔ Ask me to try to extract a minimal test case that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following target: ci-paco
  • You can also pass me a specific list of targets to minimize as arguments.

minkiminki added a commit to snu-sf/paco that referenced this pull request Dec 29, 2022
@Zimmi48 Zimmi48 linked an issue Dec 30, 2022 that may be closed by this pull request
@Zimmi48
Copy link
Member

Zimmi48 commented Dec 30, 2022

Given why hresolve_core was introduced, I don't think it should be removed without hget_evar being removed at the same time.

@SkySkimmer SkySkimmer added kind: cleanup Code removal, deprecation, refactorings, etc. request: full CI Use this label when you want your next push to trigger a full CI. and removed needs: changelog entry This should be documented in doc/changelog. labels Jan 3, 2023
@SkySkimmer SkySkimmer requested a review from a team as a code owner January 3, 2023 10:18
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jan 3, 2023
@ppedrot ppedrot self-assigned this Jan 3, 2023
@ppedrot ppedrot added this to the 8.18+rc1 milestone Jan 3, 2023
The implementation is fragile due to being based on detyping and a loc
hack, and it seems unused.

It was also not in the doc until
coq#16498 (8.17) and then only got a doc stub.
@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Jan 4, 2023
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jan 4, 2023
@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Jan 4, 2023
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jan 4, 2023
@ppedrot
Copy link
Member

ppedrot commented Jan 4, 2023

@coqbot merge now

@coqbot-app coqbot-app bot merged commit fad4cb7 into coq:master Jan 4, 2023
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jan 4, 2023

@ppedrot: Please take care of the following overlays:

  • 17035-SkySkimmer-no-hresolve.sh

@SkySkimmer SkySkimmer deleted the no-hresolve branch January 5, 2023 11:49
@herbelin
Copy link
Member

herbelin commented Jan 5, 2024

According to #16846 it is used to implement hpattern which looks like a useful tactic that I'd be interested in seeing upstreamed into Coq rather than broken

Ideas from hresolve_core were used in destruct (via Evarconv.second_order_matching). I don't remember the exact differences between Evarconv.second_order_matching and hresolve_core, but for sure, these are ideas to valorize and to develop further. Iirc, Matthieu was working on generalizing the use of Evarconv.second_order_matching as part of Unifall, but, again, I don't know the exact status of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup Code removal, deprecation, refactorings, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Are hresolve_core and hget_evar still useful?
6 participants