Remove TreeAndJoint in favor of joint field on the Token #64782
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
matklad
force-pushed
the
token-joint
branch
3 times, most recently
from
a3da6f9
to
ed58f79
Compare
matklad
changed the title
|
Should be ready for review now. Note that this causes the |
|
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 |
matklad
force-pushed
the
token-joint
branch
3 times, most recently
from
5a73379
to
a31bff6
Compare
matklad
force-pushed
the
token-joint
branch
from
a31bff6
to
42f0795
Compare
|
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) |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 42f0795 with merge 9ee6b923ba02846e7df5357e4f94b5d8e0126f0c... |
| @@ -265,10 +265,24 @@ pub enum TokenKind { | |||
| #[cfg(target_arch = "x86_64")] | |||
| 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::*;).
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).
We should be able to recover this later, the largest thing in |
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... |
|
☀️ Try build successful - checks-azure |
|
Queued 9ee6b923ba02846e7df5357e4f94b5d8e0126f0c with parent ddf4386, future comparison URL. |
|
Finished benchmarking try commit 9ee6b923ba02846e7df5357e4f94b5d8e0126f0c, comparison URL. |
|
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. |
|
@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? |
Please edit the PR description as well, that's what most people look at. |
|
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
added
S-waiting-on-author
|
The perf diff seems negligible and the change is something needs to be done anyway. |
|
@petrochenkov I think the regression may also affect macro_by_example, precisely because |
|
Ping from triage. Thank you! |
|
Yeah, I am back from dev fest Toulouse and will push this over the finish line next week |
|
Ping from triage. @matklad Hi! Just letting you know this still needs to be rebased. |
|
@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.