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 some grammar constructs. #247

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

Conversation

dj2
Copy link
Contributor

@dj2 dj2 commented Aug 25, 2024

A number of constructs in the grammar were only a single entry, or only used once (or both). This CL inlines many of these single entry constructs to the place of usage.

This makes it simpler to read the grammar requiring less jumping around to determine what is permitted.

@@ -171,6 +160,9 @@ _postfix_expression_.
endif::ESSL[]
====

_function_definition_ : ::
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one I just moved up, it was weirdly hanging out at the very end of the grammar, so I put it with some other function definitions.

@gnl21
Copy link
Contributor

gnl21 commented Sep 11, 2024

I made quite a lot of simplifying changes to the grammar a few years ago but they were rejected by the working group at the time, who didn't want to make updates to the grammar at all. I'd be very happy to see the grammar improved, because I think the current version is quite a mess, but I'd like to hear that the working group will accept changes here and to see how these changes integrate with my old changes before we merge this.

@dj2
Copy link
Contributor Author

dj2 commented Sep 11, 2024

If your old version still applies, would you be able to push it to GitHub as a PR? I'd be curious to see it. Reading the grammar now involves a lot of jumping around and confusing, un-needed entries. I'd love to see how you simplified it and am happy to drop this PR if there is another one which does something similar.

As far as I can tell, none of my changes would have any language effects, they just simplify what is currently existing.

@gnl21
Copy link
Contributor

gnl21 commented Sep 11, 2024

Sure, I'd be happy to push the changes. I'll dig them out (it's been a while). I agree about the jumping around and unneeded entries. A lot of it feels like it came from trying to manage data inside a yacc file rather than being a clear description of the language.

My changes also didn't change the language that the grammar accepted, which I think is good while the grammar is in its current state. I do think that having the grammar accept an over-broad language and then restricting it in text is a mistake though, so it might be interesting to look at that in the future.

Comment on lines -300 to -302
_function_prototype_ : ::
_function_declarator_ _RIGHT_PAREN_

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made a different change to remove this annoying rule in #252. We do need to keep function_prototype (it's referred to in function_definition, above), but if we structure it properly then this step isn't needed at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch, I'd missed that one earlier because function_definition was weirdly at the end of the grammar instead of with the other functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged it into function_defintion to keep this Cl correct, but either way these rules are very confusing to read.

A number of constructs in the grammar were only a single entry, or only
used once (or both). This CL inlines many of these single entry
constructs to the place of usage.

This makes it simpler to read the grammar requiring less jumping around
to determine what is permitted.
@arcady-lunarg
Copy link

The bison grammar in glslang follows the grammar in the GLSL spec pretty directly, so it would be nice to see an accompanying glslang change to update that grammar as well.

@gnl21
Copy link
Contributor

gnl21 commented Oct 24, 2024

I agree in general that it's nice to keep the glslang grammar in sync, if only because the new grammar should be simpler than the old one, but I don't think it should be a requirement for simplifying the documentation. We should keep the spec grammar as simple and accurate as possible because it's a powerful tool for saying what's valid, while glslang may want to differ from that for implementation reasons. (Either to simplify the code or potentially to generate better error messages where "Syntax error" would be unhelpful).

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