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

Fix pairing of functions by type. #5227

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jimblandy
Copy link
Contributor

@jimblandy jimblandy commented May 22, 2023

Change Differ::MatchFunctions to use GroupIdsAndMatchByMappedId, instead of calling GroupIdsAndMatch with Differ::GroupIdsHelperGetTypeId, which is never correct: doing so compares src and dst type ids directly, rather than consulting the established mapping.

Depends on #5225.

Fixes #5226.

@s-perron s-perron added the component:diff Issues related to spirv-diff label May 23, 2023
@s-perron
Copy link
Collaborator

@ShabbyX Can you please review this?

@ShabbyX
Copy link
Contributor

ShabbyX commented May 24, 2023

Thanks for the fix. It looks like this didn't affect the test results so it's not covered by any tests. Could you please add tests to make sure this isn't regressed in the future?

@jimblandy
Copy link
Contributor Author

jimblandy commented May 24, 2023

[EDIT]: I was confusing this PR with #5225. This is indeed not covered by tests.

Original:

@ShabbyX This one affected test/diff/diff_files/different_decorations_vertex_autogen.cpp. Is there another sort of test coverage we should have?

@jimblandy jimblandy marked this pull request as draft May 24, 2023 20:24
@jimblandy
Copy link
Contributor Author

Converted to draft until it's got test coverage.

Change `Differ::MatchFunctions` to use `GroupIdsAndMatchByMappedId`,
instead of calling `GroupIdsAndMatch` with
`Differ::GroupIdsHelperGetTypeId`, which is never correct: doing so
compares src and dst type ids directly, rather than consulting the
established mapping.

Fixes KhronosGroup#5226.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:diff Issues related to spirv-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spirv-diff probably reports spurious differences when pairing functions by type
3 participants