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

goyacc: fix unstability of generated parser.go #9

Merged
merged 2 commits into from
Nov 5, 2018

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Nov 1, 2018

Previously, running make parser two times may produce different contents in parser.go. This is because we have two pairs of tokens differed only by case: "Starting"/"starting" and "Constraint"/"constraint". goyacc by default sort the tokens by usage and lower-cased name using sort.Sort which is unstable, causing non-deterministic output. This PR fixes the issue in the parser generator by forcing the upper-case tokens be sorted before the lower-case ones.

This PR is needed for #6.

@tiancaiamao
Copy link
Collaborator

LGTM

Some bits are taken from #8 which was closed by OP.

1. Renamed the rule `goyacc` to `bin/goyacc` so Makefile won't mistaken
   the directory not changed as the binary being built
2. Modified the generated message to include the words "DO NOT EDIT."
   recommended by [`go generate`], so IDEs can properly flag the file as
   generated.

[`go generate`]: https://golang.org/pkg/cmd/go/internal/generate/
When two strings have the same lowercase value, break-even by comparing
case-sensitively.
@shenli
Copy link
Member

shenli commented Nov 5, 2018

Good catch!

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit fc47682 into master Nov 5, 2018
@zz-jason zz-jason deleted the kennytm/fix-goyacc-unstable branch November 5, 2018 13:21
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
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.

4 participants