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

astgen: fix result info for catch switch_block_err_union #19899

Merged
merged 4 commits into from
May 11, 2024

Conversation

dweiller
Copy link
Contributor

@dweiller dweiller commented May 8, 2024

Fixes #19881.

Todo

  • add behaviour tests to catch any future regressions

@dweiller dweiller marked this pull request as ready for review May 8, 2024 17:57
@Vexu Vexu enabled auto-merge (squash) May 8, 2024 18:55
@dweiller
Copy link
Contributor Author

dweiller commented May 9, 2024

Looks like the new tests as hitting a TODO in the wasm backend for lowering error union payload pointers. I'd probably need some guidance on how to implement that unless it's acceptable the skip the tests on wasm.

@Vexu
Copy link
Member

Vexu commented May 9, 2024

unless it's acceptable the skip the tests on wasm.

It is.

auto-merge was automatically disabled May 9, 2024 09:36

Head branch was pushed to by a user without write access

@dweiller dweiller force-pushed the switch-peer-type-resolution branch 2 times, most recently from ad3a846 to 33c6937 Compare May 9, 2024 09:55
@dweiller
Copy link
Contributor Author

dweiller commented May 9, 2024

Okay - I've disabled the address-of tests with the wasm backend.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

test: disable failing err union switch tests on wasm

This commit message doesn't reflect what is happening in that commit. You're making a lot of changes to behavior tests, what's going on there?

@dweiller
Copy link
Contributor Author

dweiller commented May 9, 2024

test: disable failing err union switch tests on wasm

This commit message doesn't reflect what is happening in that commit. You're making a lot of changes to behavior tests, what's going on there?

Each of the commented out function calls in test "switch on error union if else capture" would fail on wasm (I didn't check, but I'm pretty sure it will be the same reason as for the address-of tests). The commit is just cut-pasting them into the test "ptr tests" (which probably needs a better name) so they can be disabled on wasm, and unfortunately the diff algorithm doesn't seem to handle it well.

Would you prefer I simply disable the whole original "switch on error union if else capture" test?

@mlugg
Copy link
Member

mlugg commented May 10, 2024

Yeah, don't restructure the tests to favor slightly better testing of a backend which is unused in practice. Just disable the whole test for the WASM backend, and it can be re-enabled when the backend is improved.

@dweiller dweiller force-pushed the switch-peer-type-resolution branch from 994d098 to b264cc4 Compare May 10, 2024 03:44
@dweiller
Copy link
Contributor Author

Okay - I've just disabled all the tests rather than splitting apart those that did or didn't pass on wasm.

@dweiller dweiller requested a review from andrewrk May 11, 2024 08:50
@Vexu Vexu merged commit 1550b5b into ziglang:master May 11, 2024
10 checks passed
@dweiller dweiller deleted the switch-peer-type-resolution branch May 11, 2024 18: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.

incorrect peer type resolution taking the address of catch |err| switch (err)
4 participants