Remove cranelift-codegen-shared#6773
Conversation
github-actions
bot
added
cranelift
jameysharp
left a comment
jameysharp
left a comment
There was a problem hiding this comment.
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 in scripts/publish.rs? I think the verify-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.
| fmtln!( | ||
| fmt, | ||
| "pub const LANE_BASE: u16 = {:#X};\n", | ||
| constants::LANE_BASE | ||
| ); | ||
| fmtln!( | ||
| fmt, | ||
| "pub const REFERENCE_BASE: u16 = {:#X};\n", | ||
| constants::REFERENCE_BASE | ||
| ); | ||
| fmtln!( | ||
| fmt, | ||
| "pub const VECTOR_BASE: u16 = {:#X};\n", | ||
| constants::VECTOR_BASE | ||
| ); | ||
| fmtln!( | ||
| fmt, | ||
| "pub const DYNAMIC_VECTOR_BASE: u16 = {:#X};\n", | ||
| constants::DYNAMIC_VECTOR_BASE | ||
| ); |
There was a problem hiding this 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.
2 participants
Based on #6772