Skip to content
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

rustc_arena: add alloc_str #118660

Merged
merged 1 commit into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions compiler/rustc_arena/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,20 @@ impl DroplessArena {
}
}

/// Allocates a string slice that is copied into the `DroplessArena`, returning a
/// reference to it. Will panic if passed an empty string.
///
/// Panics:
///
/// - Zero-length string
#[inline]
pub fn alloc_str(&self, string: &str) -> &str {
let slice = self.alloc_slice(string.as_bytes());

// SAFETY: the result has a copy of the same valid UTF-8 bytes.
unsafe { std::str::from_utf8_unchecked(slice) }
}

/// # Safety
///
/// The caller must ensure that `mem` is valid for writes up to `size_of::<T>() * len`, and that
Expand Down Expand Up @@ -655,6 +669,14 @@ pub macro declare_arena([$($a:tt $name:ident: $ty:ty,)*]) {
self.dropless.alloc_slice(value)
}

#[inline]
pub fn alloc_str(&self, string: &str) -> &str {
if string.is_empty() {
return "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove this case and keep the panic?
No current use requires it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can if you prefer, but I think for perf the branchiness should end up equivalent, just changing whether it jumps to a panic or return. And the empty check here is consistent with its alloc_slice neighbor. A panic still makes sense if we want that treated as a bug -- is that it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In symbol.rs that would certainly be a bug, with SymbolName::new I'm less sure, but it looks like the string cannot be empty there either.
I don't have much preference here, let's land as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symbol.rs case is already using a direct DroplessArena, where it would panic. Only the macro-declared arena has the empty return case, in both alloc_slice and this new alloc_str.

}
self.dropless.alloc_str(string)
}

#[allow(clippy::mut_from_ref)]
pub fn alloc_from_iter<T: ArenaAllocatable<'tcx, C>, C>(
&self,
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2614,9 +2614,7 @@ pub struct SymbolName<'tcx> {

impl<'tcx> SymbolName<'tcx> {
pub fn new(tcx: TyCtxt<'tcx>, name: &str) -> SymbolName<'tcx> {
SymbolName {
name: unsafe { str::from_utf8_unchecked(tcx.arena.alloc_slice(name.as_bytes())) },
}
SymbolName { name: tcx.arena.alloc_str(name) }
}
}

Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2118,11 +2118,7 @@ impl Interner {
return Symbol::new(idx as u32);
}

// SAFETY: we convert from `&str` to `&[u8]`, clone it into the arena,
// and immediately convert the clone back to `&[u8]`, all because there
// is no `inner.arena.alloc_str()` method. This is clearly safe.
let string: &str =
unsafe { str::from_utf8_unchecked(inner.arena.alloc_slice(string.as_bytes())) };
let string: &str = inner.arena.alloc_str(string);

// SAFETY: we can extend the arena allocation to `'static` because we
// only access these while the arena is still alive.
Expand Down
Loading