Macros 1.1 #3064
Macros 1.1 #3064
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. |
|
Um what about the dependency side of things? Are we going with |
|
@Ericson2314 this was part of the RFC. See Initial Cargo integration and Macros 1.1 in practice. |
| .set_for_host(match (toml.plugin, toml.rustc_macro) { | ||
| (None, None) => t2.for_host(), | ||
| (Some(true), _) | (_, Some(true)) => true, | ||
| _ => false, |
There was a problem hiding this comment.
Could this explicitly bind the Some(false) here as well?
There was a problem hiding this comment.
Well out of the 9 cases, 1 is handled by the first line and 5 are handled by the second line so there are 3 left:
(None, Some(false))
(Some(false), None)
(Some(false), Some(false))Are you suggesting this?
(Some(false), _) | (_, Some(false)) => falseI think the current code is clearer. It says "if nothing is set, inherit; if anything is true, then true otherwise false." The explicitly bound Some(false) looks like it means "if anything is false, then false" but that is misleading because (Some(true), Some(false)) is true.
There was a problem hiding this comment.
Oh yes, the latter. Just writing out what the case is catching vs "something we accidentally forgot to cover above"
|
@dtolnay that's fair. I've made amore detailed comment in the RFC tracking issue—it makes more sense to discuss there than here as I'd assume you'd agree. |
|
Looking good to me! I'll look to merge once rustc support lands and we've got a nightly. |
|
I pushed an empty commit to retrigger the builds. Linux and OS X pass in Travis. Looks like AppVeyor is getting a really old nightly for some reason: rustc 1.12.0-nightly (7ad125c4e 2016-07-11). |
|
@bors: r+ Thanks! |
|
📌 Commit acf34bb has been approved by |
|
⌛ Testing commit acf34bb with merge 25d2868... |
|
💔 Test failed - cargo-linux-64 |
|
Oh I think |
|
Done. |
|
@bors: r+ |
|
📌 Commit 7ee6f57 has been approved by |
Macros 1.1 Tested with serde-rs/serde#530. This should be able to merge independently of rust-lang/rust#35957. r? @alexcrichton
|
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
Tested with serde-rs/serde#530.
r? @alexcrichton