search

KhronosGroup / GLSL

GLSL Shading Language Specification and Extensions
Other
347 stars 101 forks source link

Simplify some grammar constructs. #247

Open dj2 opened 3 months ago

dj2 commented 3 months ago

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.

gnl21 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

arcady-lunarg commented 1 month ago

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 commented 1 month ago

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).