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

Simplify glslang grammar for identifier lists #3772

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gnl21
Copy link
Contributor

@gnl21 gnl21 commented Oct 24, 2024

This was more complicated than it needed to be. The old grammar matched what was in the spec and GLSL#251 fixes it there as well.

This was more complicated than it needed to be. The old grammar matched
what was in the spec and GLSL#251 fixes it there as well.
@gnl21
Copy link
Contributor Author

gnl21 commented Oct 24, 2024

@arcady-lunarg here is one of the grammar changes to match the GLSL PRs that you requested.

The syntax errors generated by the parser have changed slightly with the
updated grammar.
@gnl21
Copy link
Contributor Author

gnl21 commented Oct 24, 2024

I've added a new commit because the error messages in the test suite have changed. They have become slightly less helpful, I think, because given

uniform foo

a LEFT_BRACE is a possible continuation (to declare a uniform block foo) but this isn't mentioned in the message anymore. I assume this is because bison has already reduced the identifier to an identifier_list, which rules out this completion.

Overall I think that this illustrates a short-coming with bison error handling rather than anything that should be adjusted in this change. Most of the occurrences in the test suite are where this is generated by having misunderstood some previous token anyway, so neither the old or the new messages make sense.

@gnl21
Copy link
Contributor Author

gnl21 commented Oct 31, 2024

The spec PR for this change has just been merged.

Copy link
Contributor

@arcady-lunarg arcady-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@arcady-lunarg
Copy link
Contributor

This will need to be rebased because of an intervening change to glslang.y, feel free to do that or to give maintainers access to push to your branch and I would be able to do it myself.

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.

2 participants