Reimplement public interface for stability#71
Conversation
|
|
||
| /// Creates a new span with the same line/column information as `self` but | ||
| /// that resolves symbols as though it were at `other`. | ||
| pub fn resolved_at(&self, other: Span) -> Span { |
There was a problem hiding this comment.
Should this and located_at be procmacro2_semver_exempt?
There was a problem hiding this comment.
I initially had that yeah but this was used by serde_derive, but I can remove the usage from serde_derive
| @@ -1,3 +1,5 @@ | |||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
Perhaps yeah, I personally find the soup of #[cfg] on imports somewhat unappealing and am generally ok with dead code otherwise, so I might personally lean towards leaving this
src/stable.rs
Outdated
| TokenNode::Group(delim, ref stream) => { | ||
| let (start, end) = match delim { | ||
| match *tt { | ||
| TokenTree::Group(ref g) => { |
There was a problem hiding this comment.
I would prefer to use the Syn match naming convention here, https://docs.rs/syn/0.12.15/syn/enum.Expr.html#syntax-tree-enums. For example TokenTree::Op(ref tt) => ... tt.op() ... to avoid op.op().
| } | ||
| } | ||
|
|
||
| #[cfg(procmacro2_semver_exempt)] |
There was a problem hiding this comment.
Ah this is the same as the #[cfg] soup above
src/lib.rs
Outdated
| } | ||
|
|
||
| pub struct TokenTreeIter(imp::TokenTreeIter); | ||
| pub struct TokenTreeIter { |
There was a problem hiding this comment.
We may want a different name here because if we ever implement IntoIterator for &TokenStream then we will want this name.
src/lib.rs
Outdated
|
|
||
| pub fn as_str(&self) -> &str { | ||
| self.0.as_str() | ||
| pub fn stream(&self) -> &TokenStream { |
There was a problem hiding this comment.
This is sort of an unfortunate signature because you can't do anything with &TokenStream except clone or check if empty. Would it work to implement IntoIterator for &TokenStream? Or is there a cheap way to return TokenStream here?
| } | ||
| } | ||
|
|
||
| pub fn set_span(&mut self, span: Span) { |
There was a problem hiding this comment.
We may need to clarify that this is a shallow set_span. Same for Group::set_span. Is that going to be confusing?
.travis.yml
Outdated
| matrix: | ||
| include: | ||
| - rust: 1.15.0 | ||
| - rust: 1.23.0 |
There was a problem hiding this comment.
Please restore support for 1.15.0.
alexcrichton
force-pushed
the
v3
branch
6 times, most recently
from
5993cd2 to
1f0e797
Compare
More information to come later about this, but this is a result of the work week discussions we've had about stabilizing procedural macros
| Literal(f.into()) | ||
| pub fn f32_unsuffixed(f: f32) -> Literal { | ||
| assert!(f.is_finite()); | ||
| Literal::_new(imp::Literal::float(f as f64)) |
There was a problem hiding this comment.
This is a bit scary, don't we want to stringify the exact f32 value? Not sure if this specific case is a problem but the compiler tried really hard (before APFloat, mostly) to avoid casting between f32 and f64.
There was a problem hiding this comment.
Yeah this'll get fixed one we update upstream to match this api
|
Aside from #71 (comment) (and a broader question of "what do we even want literals to be?"), I'm curious what the plans for bringing the built-in Specifically, this design is much nicer for the decoupling being done in rust-lang/rust#49219, so I would prefer if we could first land this new API on Should it be done in lockstep, just to avoid friction for people who are using |
|
@eddyb I plan to land this here and then land referenced prs shortly after. Once that's done I'll send a PR to Rust nightly and we can update proc-macro2 once that lands That's the current plan at least! |
|
Ok! After lots of discussion at the work week I think we've settled on this API as the best way forward for stabilizing the |
3 participants
More information to come later about this, but this is a result of the
work week discussions we've had about stabilizing procedural macros