Remove usages of Term::as_str and mark it for removal #49823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
Zoxc
force-pushed
the
term-str
branch
2 times, most recently
from
c1fa468
to
8988a63
Compare
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
src/libproc_macro/lib.rs
Outdated
| @@ -711,8 +711,8 @@ impl Term { | |||
|
|
|||
| /// Get a reference to the interned string. | |||
| #[unstable(feature = "proc_macro", issue = "38356")] | |||
| pub fn as_str(&self) -> &str { | |||
| unsafe { &*(&*self.sym.as_str() as *const str) } | |||
| pub fn to_string(&self) -> String { | |||
There was a problem hiding this comment.
If we go with this change I don't think we'd keep this as a method as it's already inherited through the ToString trait.
|
@Zoxc can you lay out the concerns motivating this as well? The links above didn't really convey to me why |
|
There isn't a mechanism to ensure that the |
dtolnay
reviewed
Apr 10, 2018
There was a problem hiding this comment.
I am on board with removing as_str in favor of the Display impl (no to_string inherent method) but unless there is an immediate soundness concern I would prefer to hold off and batch this with the next round of proc-macro2 breaking changes. We can file an issue here or in proc-macro2 to remember to follow up.
|
Ping from triage! What's the status of this PR? |
|
I think we're likely to merge this but in the meantime we'd prefer to avoid a breaking change to |
|
So, I guess this is blocked on the stabilization of |
pietroalbini
added
S-blocked
|
Hm yeah I think so. I'm gonna close this for now but we will very intentionally leave this out of the first round of stabilization. Once the first round of stabilization lands I think we're definitely safe to remove this. @Zoxc if this needs to be removed sooner though for other technical reasons we can of course go ahead and land it! |
|
@alexcrichton Is that noted down somewhere? Should I add a comment that it should not be stabilized? We could also remove the existing usages of this. |
|
@Zoxc this was incorporated into the FCP proposal, but yeah if there are existing usages it'd be great to remove them! |
|
I don't see |
|
Ah yes that's what I mean, its absence indicates that it's not being stabilized. A lot of stuff in |
Zoxc
changed the title
|
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 |
Zoxc
added
S-waiting-on-review
|
Note that this PR is obsoleted by #49219, which itself has to come up with a scheme for |
|
Ping from triage @alexcrichton! What's the status on this? |
|
@bors: r+ Should be good to go now! |
|
📌 Commit 221b7ca has been approved by |
bors
added
S-waiting-on-bors
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
May 10, 2018
Remove usages of Term::as_str and mark it for removal Returning references to rustc internal data structures is a bad idea since their lifetimes are unrelated to the lifetimes of proc_macro values. See rust-lang#46972 and the `Taming thread-local storage` section of https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606 r? @alexcrichton
bors
added a commit
that referenced
this pull request
May 10, 2018
Remove usages of Term::as_str and mark it for removal Returning references to rustc internal data structures is a bad idea since their lifetimes are unrelated to the lifetimes of proc_macro values. See #46972 and the `Taming thread-local storage` section of https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606 r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Returning references to rustc internal data structures is a bad idea since their lifetimes are unrelated to the lifetimes of proc_macro values.
See #46972 and the
Taming thread-local storagesection of https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606r? @alexcrichton