Uh oh!
There was an error while loading. Please reload this page.
Fix lifetime on LocalInternedString::get function - #59227
Conversation
There was a problem hiding this comment.
Re-interning here was intentional (to get rid of possible gensyms before checking).
There was a problem hiding this comment.
Hm, shouldn't this keep working with non-static lifetime?
Also, a typo "f64f64" below.
There was a problem hiding this comment.
No, the LocalInternedString is dropped in the first statement here.
There was a problem hiding this comment.
I assume this is going to ruin the fn ident_str API in #58899 then.
Treating an attribute name as &str (including matching on it, calling contains) is a common operation, so returning Option<LocalInternedString> instead and mapping on it every time would be a serious usability hit.
There was a problem hiding this comment.
Yeah. fn ident_str won't work. We should really use symbols for attribute names instead of comparing strings all the time.
There was a problem hiding this comment.
True, the issue is just that a lot of existing code across rustc uses strings.
There was a problem hiding this comment.
Please add a comment and maybe an ui-fulldeps test, to ensure this doesn't happen again?
And maybe self.string should be *const str instead.
There was a problem hiding this comment.
I made this use unsafe code. That should prevent people from modifying this without reading comments.
bors
commented
Mar 17, 2019
☔ The latest upstream changes (presumably #58899) made this pull request unmergeable. Please resolve the merge conflicts. |
Centril
commented
Mar 17, 2019
r? @eddyb |
rust-highfive
commented
Mar 20, 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 |
nnethercote
commented
Mar 20, 2019
Thanks for fixing this, @Zoxc. It's certainly an interesting lesson for me in the potential non-local effects of |
There was a problem hiding this comment.
IMO one should write *other rather than .deref().
There was a problem hiding this comment.
Actually, isn't AsRef<str> common practice in these sort of situations?
There was a problem hiding this comment.
There are too many of these, maybe a macro should be used (which can do *other == *self, I think).
eddyb
commented
Mar 21, 2019
r=me with nits fixed (if at all, they're kind of inconsequential) |
There was a problem hiding this comment.
I just realized this should probably be NonNull<str>.
rust-highfive
commented
Mar 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 |
Centril
commented
Mar 30, 2019
Ping from triage, @Zoxc |
There was a problem hiding this comment.
Argh, all the derives are now wrong...
There was a problem hiding this comment.
I just changed this back to &'static str.
rust-highfive
commented
Mar 31, 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 |
eddyb
commented
Apr 10, 2019
@bors r+ |
bors
commented
Apr 10, 2019
📌 Commit 5ea959d has been approved by |
bors
commented
Apr 10, 2019
🌲 The tree is currently closed for pull requests below priority 15, this pull request will be tested once the tree is reopened |
Fix lifetime on LocalInternedString::get function cc @eddyb@nnethercote
bors
commented
Apr 11, 2019
Fix lifetime on LocalInternedString::get function cc @eddyb@nnethercote
bors
commented
Apr 12, 2019
☀️ Test successful - checks-travis, status-appveyor |
rust-highfive
commented
Apr 12, 2019
📣 Toolstate changed by #59227! Tested on commit cd8b437. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth@llogiq@mcarton@oli-obk@phansch, @rust-lang/infra). |
Tested on commit rust-lang/rust@cd8b437. Direct link to PR: <rust-lang/rust#59227> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth@llogiq@mcarton@oli-obk@phansch, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth@llogiq@mcarton@oli-obk@phansch, @rust-lang/infra). 💔 miri on windows: test-fail → build-fail (cc @oli-obk@RalfJung@eddyb, @rust-lang/infra). 💔 miri on linux: test-fail → build-fail (cc @oli-obk@RalfJung@eddyb, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc@Xanewok, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc@Xanewok, @rust-lang/infra).
Rustup cc rust-lang/rust#59227 (comment) This fix is obsolet once rust-lang/rust#59779 and #3926 is merged.
cc @eddyb@nnethercote