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

[wgsl-in] Split into multiple files #2207

Merged
merged 6 commits into from
Jan 31, 2023
Merged

Conversation

SparkyPotato
Copy link
Contributor

@SparkyPotato SparkyPotato commented Jan 14, 2023

In #2075, we left all the changes in one file to make reviewing easier. However, this made front/wgsl/mod.rs huge (5,447 lines), and quite challenging to follow.

This PR splits each pass into its own module, bringing both parse/mod.rs and lower/mod.rs to a more manageable ~2,000 lines. Other than uses, everything has been copy-pasted.

Since the frontends now do more than parse, I also renamed them to Frontend instead of Parser to make the intent more clear: #2075 (comment)

@jimblandy
Copy link
Member

This looks great, but let's fix those doc links.

@SparkyPotato
Copy link
Contributor Author

Oops, missed that.

Rustdoc seems to be acting weird, however.
Linking crate::front::wgsl::lower::ExpressionContext::local_table directly errors, but if I use crate::front::wgsl::lower::ExpressionContext, it stops complaining.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This looks good, just some minor points.

Acknowledging Teo's suggestion in #2075 that all the front ends should use Frontend instead of Parser, I have to say I would also approve a PR that just left GLSL alone.

src/front/glsl/mod.rs Outdated Show resolved Hide resolved
src/front/glsl/builtins.rs Outdated Show resolved Hide resolved
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just minor comments.

I don't know how @JCapucho is going to feel about the GLSL changes. If he's not happy with them, we can back them out.

src/front/wgsl/error.rs Outdated Show resolved Hide resolved
src/front/wgsl/parse/mod.rs Show resolved Hide resolved
src/front/wgsl/lower/mod.rs Show resolved Hide resolved
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

@jimblandy
Copy link
Member

Clippy got clevererer.

@jimblandy
Copy link
Member

Filed #2229 for the Clippy problems.

@jimblandy
Copy link
Member

rebased on merged #2229

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