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

Glsl error message improvements #934

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

jakobhellermann
Copy link
Contributor

Before:
image
After:
image

@jakobhellermann jakobhellermann force-pushed the glsl-error-improvements branch from a413c88 to b5814b6 Compare June 1, 2021 19:57
Copy link
Collaborator

@JCapucho JCapucho left a comment

Choose a reason for hiding this comment

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

Besides my personal hate for error reporting in libraries there are some problems especially with the expected case in the InvalidToken error (which should probably be renamed to UnexpectedToken), great work nonetheless :)

Also I think that in the long run we should rename ParseError to Error and add a meta field to it so that all errors are forced to have metadata information, but that's for another PR.

Cargo.toml Outdated Show resolved Hide resolved
bin/naga.rs Outdated Show resolved Hide resolved
src/front/glsl/error.rs Outdated Show resolved Hide resolved
src/front/glsl/error.rs Outdated Show resolved Hide resolved
src/front/glsl/error.rs Outdated Show resolved Hide resolved
src/front/glsl/error.rs Outdated Show resolved Hide resolved
src/front/glsl/error.rs Outdated Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor Author

I personally don't like the library being responsible for error reporting

I'm fine with removing the codespan reporting from this PR and having it just provide more context to the errors, I agree that it isn't really the responsibility of the library.

Currently the wgsl frontend also has emit_to_stderr and emit_to_string functions, do you think they should be moved to bin/naga.rs instead?

@JCapucho
Copy link
Collaborator

JCapucho commented Jun 1, 2021

I personally don't like the library being responsible for error reporting

I'm fine with removing the codespan reporting from this PR and having it just provide more context to the errors, I agree that it isn't really the responsibility of the library.

Currently the wgsl frontend also has emit_to_stderr and emit_to_string functions, do you think they should be moved to bin/naga.rs instead?

No need to remove it, it's just my personal opinion and the other maintainers already showed support for it being included in the library and it's already included in wgsl as you said but I think we should at least make it optional since some end users may only want to compile their already known working shaders and bringing in another dependency would only increase their compile times with no real benefit

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Jun 2, 2021

Thank you! Changes look reasonable. Let's get @JCapucho having another look.

@JCapucho
Copy link
Collaborator

JCapucho commented Jun 2, 2021

I would like to fix the way the expected tokens are handled in invalid token before merging

@jakobhellermann jakobhellermann force-pushed the glsl-error-improvements branch from 17fdb9d to e915b0f Compare June 13, 2021 09:12
Copy link
Collaborator

@JCapucho JCapucho left a comment

Choose a reason for hiding this comment

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

I'm sorry I didn't express my intent clearly but when I meant a list of tokens I was talking about a literal Vec<Token>

src/front/glsl/error.rs Outdated Show resolved Hide resolved
src/front/glsl/parser.rs Outdated Show resolved Hide resolved
@jakobhellermann jakobhellermann force-pushed the glsl-error-improvements branch 2 times, most recently from bccd6a8 to 1dee90e Compare June 18, 2021 08:45
@JCapucho
Copy link
Collaborator

Looks good to me, let's just have @kvark take a look, I'm going to squash this afterward unless you want to rebase the broken commits with the ci fixes

@jakobhellermann jakobhellermann force-pushed the glsl-error-improvements branch from 480d14c to 1338361 Compare June 18, 2021 09:51
@jakobhellermann
Copy link
Contributor Author

I rebased the commits.

Copy link
Collaborator

@JCapucho JCapucho left a comment

Choose a reason for hiding this comment

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

I've double checked the PR and found some small things but we can merge it anyways and fix it later

src/front/glsl/parser.rs Outdated Show resolved Hide resolved
src/front/glsl/parser.rs Outdated Show resolved Hide resolved
src/front/glsl/variables.rs Outdated Show resolved Hide resolved
@jakobhellermann jakobhellermann force-pushed the glsl-error-improvements branch from 476be42 to 917a2f2 Compare June 18, 2021 14:52
@JCapucho
Copy link
Collaborator

Please fix CI and if possible squash the last commit and I'll merge this

@jakobhellermann jakobhellermann force-pushed the glsl-error-improvements branch from 917a2f2 to 6b75cdd Compare June 18, 2021 15:13
@JCapucho JCapucho merged commit 9d7e5cf into gfx-rs:master Jun 18, 2021
@JCapucho
Copy link
Collaborator

Thank you for your contribution

@jakobhellermann jakobhellermann deleted the glsl-error-improvements branch June 18, 2021 15:24
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.

4 participants