Rewrite and Fix Remaining Padding Bugs #35
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LPGhatguy
changed the title
|
Success! I found three bugs in Naga doing this:
|
|
Looking good, time to merge and move onto the next round of work. |
leod
pushed a commit
to leod/crevice
that referenced
this pull request
Jun 3, 2023
* Cleaner validation tests * Introduce memoffset * New tests using manual offset checking * yeah that test is broken too * Rename AsStdX::StdXType to AsStdX::Output for sanity * Rename test file * Revamp and merge test suites * Squish tests more, make wgpu portion not run by default * Rewrite Crevice from the ground up, suddenly passing all the tests * Add primitive test for mat3 * Add a bare mat4 test * Fix std430 derive and add it to tests * Use actual struct alignment instead of min alignment for trailing padding * Start testing (and fix) std430 * Add and disable tests to work around wgpu
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #29.
It's been awhile since I've dug into this problem. In this PR, I rewrote Crevice almost completely to fix all the remaining layout bugs and tighten up the test suite to be much more effective.
I think that some of our snapshot tests ended up incorrect for whatever reason, so I've replaced them with a macro that generates
offset_of!assertions. I think this will be much more clear and encourage test authors to read through the spec and reason about results instead of committing snapshots as-is.Generated structs now have padding after each field instead of before. This lets us pad at the end of structs consistently. As part of that refactor, I improved the clarity of the derive macro code dramatically. It should be a lot easier to follow.
Also done during this PR is tightening of names with the crate's traits. The associated type on
AsStd{140,430}is now calledOutput. I may rename these traits toIntoStd{140,430}to make them less semantically awkward, and then propagate that through all the method names.All in all, this implementation has little in common with the old one and hopefully has fewer bugs.