Stream: git-wasmtime

Topic: wasmtime / PR #9492 test-macros: Handle dead code warnings


view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera opened PR #9492 from saulecabrera:test-macros-handle-dead-code-warnings to bytecodealliance:main:

This commit handles dead code warnings produced by the compiler when the macro is invoked as:

#[wasmtime_test(strategies(not(Cranelift))]

Which occur because Winch currently only offers support for x86_64.

The motivation behind this change comes from
https://github.com/bytecodealliance/wasmtime/pull/9490, which is where the warnings surfaced.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera requested elliottt for a review on PR #9492.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera requested wasmtime-core-reviewers for a review on PR #9492.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera requested alexcrichton for a review on PR #9492.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 14:48):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 14:48):

alexcrichton created PR review comment:

As an alternative instead of #[cfg(target_arch = "x86_64")] being emitted, how about #[cfg_attr(target_arch = "x86_64", ignore)]? That way we wouldn't have to worry about unused warnings or dead code I think

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 15:25):

saulecabrera created PR review comment:

I briefly tried this approach, I didn't continue down this route because the compiler was complaining about multiple cfg predicates are specified, presumably because some tests have other extra attributes like #[cfg_attr(miri, ignore)] -- I think we could parse those attributes and modify them, however, since this is "temporary" (i.e., once we can fully test aarch64) I thought it might make sense to go with this approach.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 15:25):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 15:27):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 15:27):

alexcrichton created PR review comment:

Ah ok makes sense yeah. I wonder though, could we take a leaf out of the book of tests/wast.rs and do #[cfg_attr(..., should_fail)] instead? That way we're guaranteed to run the test and we just assert that it's an error instead of passing.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 17:06):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 17:06):

saulecabrera created PR review comment:

Had an offline discussion and concluded that should_panic probably won't play nicely with tests that return Result, so we're moving forward with the current approach.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 17:21):

saulecabrera merged PR #9492.


Last updated: Dec 23 2024 at 12:05 UTC