-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix lifetime on LocalInternedString::get function #59227
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -524,7 +524,11 @@ impl LocalInternedString { | |
} | ||
} | ||
|
||
pub fn get(&self) -> &'static str { | ||
pub fn get(&self) -> &str { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment and maybe an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this use unsafe code. That should prevent people from modifying this without reading comments. |
||
// This returns a valid string since we ensure that `self` outlives the interner | ||
// by creating the interner on a thread which outlives threads which can access it. | ||
// This type cannot move to a thread which outlives the interner since it does | ||
// not implement Send. | ||
self.string | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, shouldn't this keep working with non-static lifetime?
Also, a typo "f64f64" below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the
LocalInternedString
is dropped in the first statement here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, callingcontains
) is a common operation, so returningOption<LocalInternedString>
instead andmap
ping on it every time would be a serious usability hit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, the issue is just that a lot of existing code across rustc uses strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #59256