bjorn3 opened PR #6773 from bjorn3:cleanup3
to bytecodealliance:main
:
Based on https://github.com/bytecodealliance/wasmtime/pull/6772
bjorn3 requested wasmtime-compiler-reviewers for a review on PR #6773.
bjorn3 requested abrown for a review on PR #6773.
bjorn3 requested wasmtime-default-reviewers for a review on PR #6773.
elliottt requested jameysharp for a review on PR #6773.
bjorn3 updated PR #6773.
jameysharp submitted PR review:
I discussed this today with @cfallin and @fitzgen and we're in favor of these changes. Reducing the time it takes to build Cranelift is super helpful. I was a little uncomfortable with adding more strings we have to emit from codegen-meta but the change is well-motivated and you've done a great job making the result as easy to maintain as possible. Thanks!
I think one further change is necessary before we can merge this: don't we need to remove
cranelift-codegen-shared
from both lists inscripts/publish.rs
? I think theverify-publish
CI job would have caught this but it was skipped.Thanks also to @elliottt for reminding me that I hadn't responded to this PR yet.
jameysharp submitted PR review:
I discussed this today with @cfallin and @fitzgen and we're in favor of these changes. Reducing the time it takes to build Cranelift is super helpful. I was a little uncomfortable with adding more strings we have to emit from codegen-meta but the change is well-motivated and you've done a great job making the result as easy to maintain as possible. Thanks!
I think one further change is necessary before we can merge this: don't we need to remove
cranelift-codegen-shared
from both lists inscripts/publish.rs
? I think theverify-publish
CI job would have caught this but it was skipped.Thanks also to @elliottt for reminding me that I hadn't responded to this PR yet.
jameysharp created PR review comment:
I don't feel strongly about this, but I'd be tempted to use a macro here to ensure that the name of the constant is always the same in the generated source as it is in the meta crate. Something like
constants!(fmt, LANE_BASE, REFERENCE_BASE, ...)
I guess.I'm happy to merge this as-is, but I thought I'd mention it since I think
publish.rs
needs to be fixed anyway. Let me know whether you want to change this too.
Last updated: Dec 23 2024 at 13:07 UTC