Make it possible to construct TargetInfo without Context #5355
Conversation
Favor accessing cx.build_config directly.
Favor accessing cx.build_config directly.
Favor accessing cx.build_config directly.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| cx.build_config | ||
| .requested_target | ||
| .as_ref() | ||
| .map(|s| s.as_ref()), |
There was a problem hiding this comment.
Could we make some of this fields into methods of build_config, so that the callers need not to worry about moving out of it and turning an &Option<String> into Option<&str>? Could we make some of BuildConfig fields private?
There was a problem hiding this comment.
For requested_target, sure, I think such a method could make sense. As for making the fields private, I'm not really sure what the point would be? It looks like it was defined very much just as a bag of properties, and the stuff in there is straightforward enough (and typed in such a way) that there doesn't seem to be much downside to just exposing them like they are now. (I should mention that my previous language of choice was Python, where the "consenting adults" philosophy very much reigned supreme, so my take might be more controversial if you're coming from C++ or Java.)
There was a problem hiding this comment.
I have no strong opinions about this, but to me "using only fields" or "using only methods" seems slightly superior to "use fields for some stuff and methods for some other stuff",
Code like
Kind::Host => &cx.build_config.host_triple,
Kind::Target => cx.build_config.target_triple(),
seems slightly funny, and makes you pause for a split second.
And looks like after #5354 we might need less public access to fields?
But, as I've said, it's an ultra minor issue :)
There was a problem hiding this comment.
If you want to go all or none I would leave it as none. :) After replacing all instances of .requested_target with a method .requested_target() (a diff which in itself is +13/-13), there are 38 instances of build_config\., about 14 of which are method calls, so that's a slight minority.
| @@ -150,7 +168,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> { | |||
| files: None, | |||
| }; | |||
|
|
|||
| cx.probe_target_info()?; | |||
| cx.compilation.host_dylib_path = cx.host_info.sysroot_libdir.clone(); | |||
| cx.compilation.target_dylib_path = cx.target_info.sysroot_libdir.clone(); | |||
There was a problem hiding this comment.
Should we move this into Compilation::new as well?
There was a problem hiding this comment.
I definitely thought about it. I decided against it for now mostly on style (passing around long names is kind of ugly). I just looked into how the rest of Compilation gets initialized, and it looks like pretty much everything gets initialized after the fact from throughout Context, so passing these two in at new() is not a win for consistency. What do you think?
|
📌 Commit 3ec14e0 has been approved by |
Make it possible to construct TargetInfo without Context This should make stuff like #5349 easier.
|
@matklad and thank you for the quick and thorough review! |
|
☀️ Test successful - status-appveyor, status-travis |
This should make stuff like #5349 easier.