Uh oh!
There was an error while loading. Please reload this page.
Remove TreeAndJoint in favor of joint field on the Token - #64782
Conversation
rust-highfive
commented
Sep 25, 2019
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
commented
Sep 25, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
a3da6f9 to
ed58f79Comparematklad
commented
Sep 26, 2019
Should be ready for review now. Note that this causes the |
rust-highfive
commented
Sep 26, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
5a73379 to
a31bff6Comparematklad
commented
Sep 26, 2019
I wonder if we should go further already in this PR, and push jointness further down, to the relevant |
r? @petrochenkov cc @nnethercote (on the size change) |
petrochenkov
commented
Sep 26, 2019
@bors try @rust-timer queue |
rust-timer
commented
Sep 26, 2019
Awaiting bors try build completion |
bors
commented
Sep 26, 2019
⌛ Trying commit 42f0795 with merge 9ee6b923ba02846e7df5357e4f94b5d8e0126f0c... |
| static_assert_size!(TokenKind, 16); | ||
| #[derive(Clone, Copy, PartialEq, RustcEncodable, RustcDecodable, Debug)] | ||
| pub enum IsJoint { |
There was a problem hiding this comment.
This can almost reuse proc_macro::Spacing, except that it doesn't implement Rustc{Encodable,Decodable}.
Perhaps it can still be done if the *codable traits are implemented for Token manually?
There was a problem hiding this comment.
I definitely think that we should at least rename this to Spacing. As for reusing the actual type, I don't know. In general, I feel that that a boilerplaty conversion on the boundary between two systems is better than sharing actual types, as it is more resilient to changes.
There was a problem hiding this comment.
At least proc_macro::Spacing is stable and won't change though.
There was a problem hiding this comment.
except that it doesn't implement Rustc{Encodable,Decodable}
Actually the impls can be added to libserialize, like it's done for some libstd types used in the compiler in https://github.com/rust-lang/rust/blob/master/src/libserialize/collection_impls.rs.
There was a problem hiding this comment.
Anyway, at least the naming should be consistent with proc_macro::Spacing (and let's also remove pub use IsJoint::*;).
petrochenkov
commented
Sep 26, 2019
I don't think that's a good idea, we should probably track jointness for all tokens, even if we don't expose it for all token kinds in the proc macro API. |
| self.real_token(); | ||
| let is_joint = self.joint_to_prev == Joint && self.token.is_op(); | ||
| Ok((tt, if is_joint { Joint } else { NonJoint })) | ||
| let is_joint = self.joint_to_prev == Joint && token.is_op() && self.token.is_op(); |
There was a problem hiding this comment.
Perhaps it's time to move the is_op check to the proc macro API boundary (and track precise jointness for everything internally)?
There was a problem hiding this comment.
Makes sense. I've modified this line to avoid modifying a unit test, but I don't see why can't we move it entirely to proc-macro layer. (although I'd rather do it in a separate PR).
petrochenkov
commented
Sep 26, 2019
We should be able to recover this later, the largest thing in |
matklad
commented
Sep 26, 2019
I am not entirely sure that this is a good idea. My end goal here is to use exactly the same token trees as proc macros. And, in proc-macro model, suffix is a part of a literal, and not a separate joint token. OTOH, due to the way macro_rules work, we might be unable to move to proc macro model fully anyway... |
bors
commented
Sep 26, 2019
☀️ Try build successful - checks-azure |
rust-timer
commented
Sep 26, 2019
Queued 9ee6b923ba02846e7df5357e4f94b5d8e0126f0c with parent ddf4386, future comparison URL. |
rust-timer
commented
Sep 27, 2019
Finished benchmarking try commit 9ee6b923ba02846e7df5357e4f94b5d8e0126f0c, comparison URL. |
nnethercote
commented
Sep 27, 2019
Slight increases in instruction counts, up to 0.5%. Unsurprising that There is no description of the motivation for this change, either on the PR or the commit. I've looked at the commit and see that it moves a bunch of stuff around, but the benefits of doing that aren't clear to me. @matklad, you said above "My end goal here is to use exactly the same token trees as proc macros." Is that the motivation? Can you explain that some more? Thanks. |
matklad
commented
Sep 27, 2019
@nnethercote sorry for the confusing description. This is a step towards #63689. I put this into the comment in the code, but should have duplicated this into the commit description as well, will do with the next rebase. The TL;RD of that issue is that rustc represents Moreover, at the moment I don't know whether the refactoring here is worth the perf loss. Is there perhaps some easy way to recover ground here? |
eddyb
commented
Sep 27, 2019
Please edit the PR description as well, that's what most people look at. |
nnethercote
commented
Sep 27, 2019
I suspect the |
| Token { kind, span, joint: NonJoint } | ||
| } | ||
| crate fn with_joint(self, joint: IsJoint) -> Self { |
There was a problem hiding this comment.
From the uses of this function it seems like a usual constructor fn with_spacing(TokenKind, Span, Spacing) -> Self would fit better.
petrochenkov
commented
Sep 27, 2019
The perf diff seems negligible and the change is something needs to be done anyway. |
matklad
commented
Sep 27, 2019
@petrochenkov I think the regression may also affect macro_by_example, precisely because |
JohnCSimon
commented
Oct 5, 2019
Ping from triage. Thank you! |
matklad
commented
Oct 5, 2019
Yeah, I am back from dev fest Toulouse and will push this over the finish line next week |
JohnCSimon
commented
Oct 12, 2019
Ping from triage. @matklad Hi! Just letting you know this still needs to be rebased. |
nnethercote
commented
Oct 15, 2019
matklad
commented
Oct 15, 2019
@nnethercote thanks for the heads up! I turned out to be pretty busy this moment, so please go ahead with your other changes, and I'll rebase this work when I come back to this. Closing for know, but I am keeping one eye on it! |
This is a step towards #63689.
The TL;RD of that issue is that rustc represents >> as a single token at the moment, while proc_macros represent it as a pair of tokens (>, Joint), (>, _). And we want to move the parser to proc_macro representation of tokens, with the two main motivations being: a) don't having two things in the compiler, b) making parser's interface more obviously right, to help with extracting the parser into a separate library.
Moreover, at the moment rustc actually tracks jointness in two ways in a single data structure. TokenStream, before this PR stores (TokenTree, IsJoin) pairs, while the TokenTree itself can be a composite or decomposed token. And this jointness info is pretty easily lost via this impl. This PR by itself doesn't solve the two reprs problem, but it does help with not accidentally loosing jointness. In particular, macros by example now preserve jointness, while they erased it previously.