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

Fixes #17583: regression using numerals in the syntax of tactic notations #17589

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented May 9, 2023

Fixes / closes #17583.

We are a bit less eager to define every terminal of a Tactic Notation as a keyword, following what we already did for Notation.

We make a patch also for Ltac2 Notation which had the same limitation (Ltac2 developers: tell if this is ok).

  • Added / updated test-suite.
  • Added changelog.

@herbelin herbelin added kind: fix This fixes a bug or incorrect documentation. part: tactics part: notations The notation system. request: full CI Use this label when you want your next push to trigger a full CI. labels May 9, 2023
@herbelin herbelin added this to the 8.17.1 milestone May 9, 2023
@herbelin herbelin requested review from a team as code owners May 9, 2023 18:45
@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 May 9, 2023
@ppedrot
Copy link
Member

ppedrot commented May 9, 2023

As a superficial comment, this looks very hackish to me, even for Ltac1.

@herbelin
Copy link
Member Author

You mean that the criterion for deciding when a keyword is needed is hackish? In some sense, yes, it is hackish counterpart of not having a more local treatment of keywords.

Regarding the code, otherwise, I would say that it should be factorized in one central place (except that GADT and different Term/NonTerm types make it difficult to factorize).

@SkySkimmer SkySkimmer requested review from a team and removed request for a team May 12, 2023 11:12
@Zimmi48 Zimmi48 removed this from the 8.17.1 milestone Jun 27, 2023
@SkySkimmer
Copy link
Contributor

@ppedrot @herbelin so should we merge or not?

plugins/ltac2/tac2entries.ml Outdated Show resolved Hide resolved
vernac/egramml.ml Outdated Show resolved Hide resolved
vernac/egramml.ml Outdated Show resolved Hide resolved
@herbelin herbelin force-pushed the master+fix17583-regression-using-numerals-in-tactic-notation branch from 2e79185 to b0091de Compare February 29, 2024 07:40
@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 Feb 29, 2024
herbelin added a commit to herbelin/github-coq that referenced this pull request Feb 29, 2024
@herbelin herbelin force-pushed the master+fix17583-regression-using-numerals-in-tactic-notation branch from b0091de to cc04085 Compare February 29, 2024 07:46
After an ident, it does not need (see Metasyntax.make_production for
the similar computation in constr): after another terminal, it does not need.
@herbelin herbelin force-pushed the master+fix17583-regression-using-numerals-in-tactic-notation branch from cc04085 to 1f13c98 Compare February 29, 2024 11:46
@SkySkimmer
Copy link
Contributor

What are we doing with this?

@herbelin
Copy link
Member Author

I answered @ppedrot's remark I think. Now, the PR seen as fixing a regression passed over 3 versions probably, so maybe not any more so important either.

@ppedrot
Copy link
Member

ppedrot commented May 23, 2024

Let's close thus.

@ppedrot ppedrot closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. part: notations The notation system. part: tactics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntax error: [natural] expected after 'Extern' (in [hint]).
4 participants