feat: Simplify impl using `strum_macros` & impl more traits

[NobodyXu]

I've replaced many manual implementation of FromStr and Display with strum_macros::{EnumString, Display} which makes this crate easier to maintain.

• Impl From<*Architecture> for &'static str
• Impl AsRef<str> for *Vendor
• Impl OperatingSystem::into_string
• Impl From<Environment> for &'static str
• Impl From<BinaryFormat> for &'static str

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

[NobodyXu]

I've checked the generated code using cargo expand targets and strum_macros generates the correct output.

[bjorn3]

Cranelift currently doesn't use any proc macros to keep compile times in check. strum is a proc macro.

[sunfishcode]

While there may be no targets today which need a dynamic string anywhere other than the Vendor today, target triples are not a centrally defined format, so it's difficult to make predictions about what might happen in the future. We can't guarantee that any of the types will always be able to do Into<&'static str> or even just AsRef<str>. So, we could add these conversions now, and plan to do a semver bump if we ever need to remove them. But on the other hand, could you say more about the use case that motivates adding these conversions?

Also, as @bjorn3 mentions, strum-macros depends on proc macros and syn, which makes it a pretty heavy build-time dependency. As long as we can keep the code factored to only have one copy of each string in the code, it isn't too much work to maintain, and I think it's worth the little extra maintenance effort to keep build times for our users low.

Impl OperatingSystem::into_string

This one is redundant because we already have Display impls which provide .to_string().

[NobodyXu]

Cranelift currently doesn't use any proc macros to keep compile times in check. strum is a proc macro.

Yeah but using proc macro really removes a lot of code and I think it won't blow up the compile time. With that said, if you want to compile cranelift as fast as possible, then using proc macro indeed doesn't make sense.

But on the other hand, could you say more about the use case that motivates adding these conversions?

I plan to use this in cargo-binstall to render a few templates and I'd prefer return a borrowed string instead of allocating a String.

But it is just something good to have, if you think it's unnecessary I can close this PR.

This one is redundant because we already have Display impls which provide .to_string().

Well OperatingSystem::into_string returns Cow<'static, str> since in most cases it just returns a 'static str.

[sunfishcode]

On the laptop I'm typing this on, cargo build in a clean target-lexicon tree takes 0.88 seconds. With this PR in its current form, it takes 4.56 seconds. That's a significant difference for developers in our downstream crates.

Are you printing these values with format! or similar? If so, then it isn't constructing temporary strings for them, because it calls write_str to write the string into the formatter rather than returning a string.

Well OperatingSystem::into_string returns Cow<'static, str> since in most cases it just returns a 'static str.

Ah, I missed that. If you do need to return these as strings, what would you think of having all these conversions use this approach of Cow<'static, str>?

[NobodyXu]

On the laptop I'm typing this on, cargo build in a clean target-lexicon tree takes 0.88 seconds. With this PR in its current form, it takes 4.56 seconds. That's a significant difference for developers in our downstream crates.

I tested on my M1 and I get similar results on dev-build. It indeed hits

Are you printing these values with format! or similar? If so, then it isn't constructing temporary strings for them, because it calls write_str to write the string into the formatter rather than returning a string.

I'm using leon::Values for rendering so I was thinking if it is possible to avoid string allocation altogether.

Ah, I missed that. If you do need to return these as strings, what would you think of having all these conversions use this approach of Cow<'static, str>?

That is definitely more forward compatible and is acceptible in my use case.

[NobodyXu]

@sunfishcode I've implemented what you requested in a new PR #90