Create newtype for platform triple in bootstrap. #36696
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| self.0.split('-').next().unwrap() | ||
| } | ||
|
|
||
| pub fn is_msvc(&self) -> bool { |
There was a problem hiding this comment.
Can some of these conditionals be combined? I was very conservative about not attempting to merge any.
| /// * `i686-apple-darwin` | ||
| /// * `powerpc-unknown-linux-gnu` | ||
| #[derive(Clone, Debug, Default, Eq, Hash, PartialEq, RustcDecodable)] | ||
| pub struct Triple(pub String); |
There was a problem hiding this comment.
In the future, maybe this could be an enum that has all the platforms we support?
There was a problem hiding this comment.
I don't think that would work due to the flexible / json target specifications.
|
☔ The latest upstream changes (presumably #36442) made this pull request unmergeable. Please resolve the merge conflicts. |
As I was reading through the bootstrap code, I noticed myself getting lost at times trying to find out what the value of each `&str` is. I also noticed there's quite a few platform-checking conditionals checking the contents of these strings. In an attempt to clarify and cleanup these cases, I created a 'newtype' `Triple` that represents a platform triple and moved the platform-checking logic to methods on the new struct.
frewsxcv
force-pushed
the
bootstrap-target
branch
from
88b6f1f to
75d141c
Compare
|
Rebased. |
|
Thanks for the PR! I've been somewhat hesitant to do this in the past though as I've not been super certain about what the benefit is and being wary of over-abstracting. This is already something that all build scripts have to deal with (e.g. they don't have a shared Could you elaborate though on some of the confusion you saw? I wonder if perhaps a |
|
When I see a Anyways, I don't feel strongly about this, mostly just throwing around patterns I personally find helpful. |
|
Hm yeah it's true that |
Just to clarify, which build scripts are you referring to? |
|
Oh just build scripts in general for Cargo crates, which the build system is somewhat intended to follow after |
|
I'm not seeing anything about a target triple in the build script documentation, so I'm still not entirely sure what you're referring to here. Regardless, it doesn't sound like there's much enthusiasm around this change, so I'm leaning towards closing this. I'm curious to know your thoughts about the 'newtype' pattern since to me, this seemed like a good use of it. |
|
Oh sorry I just meant env vars like |
|
I'm personally not a huge fan of newtypes as extra types in the limit just ends up forcing everyone to juggle a huge number of types in their heads, but when applied well they can certainly work nicely! |
|
Sounds good, thanks for the input! |
As I was reading through the bootstrap code, I noticed myself getting
lost at times trying to find out what the value of each
&stris. Ialso noticed there's quite a few platform-checking conditionals checking
the contents of these strings. In an attempt to clarify and cleanup
these cases, I created a 'newtype'
Triplethat represents a platformtriple and moved the platform-checking logic to methods on the new
struct.