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.

Sign up for GitHub

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

Jump to bottom

Improve the readability of List<T>. #91617

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Improve the readability of List<T>. #91617

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
140 changes: 89 additions & 51 deletions compiler/rustc_middle/src/ty/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use crate::arena::Arena;

use rustc_serialize::{Encodable, Encoder};

use std::alloc::Layout;
use std::cmp::Ordering;
use std::fmt;
Expand All @@ -12,49 +10,69 @@ use std::ops::Deref;
use std::ptr;
use std::slice;

extern "C" {
/// A dummy type used to force `List` to be unsized while not requiring references to it be wide
/// pointers.
type OpaqueListContents;
}

/// A wrapper for slices with the additional invariant
/// that the slice is interned and no other slice with
/// the same contents can exist in the same context.
/// This means we can use pointer for both
/// equality comparisons and hashing.
///
/// Unlike slices, the types contained in `List` are expected to be `Copy`
/// and iterating over a `List` returns `T` instead of a reference.
///
/// Note: `Slice` was already taken by the `Ty`.
/// `List<T>` is a bit like `&[T]`, but with some critical differences.
/// - IMPORTANT: Every `List<T>` is *required* to have unique contents. The
/// type's correctness relies on this, *but it does not enforce it*.
/// Therefore, any code that creates a `List<T>` must ensure uniqueness
/// itself. In practice this is achieved by interning.
/// - The length is stored within the `List<T>`, so `&List<Ty>` is a thin
/// pointer.
/// - Because of this, you cannot get a `List<T>` that is a sub-list of another
/// `List<T>`. You can get a sub-slice `&[T]`, however.
/// - `List<T>` can be used with `CopyTaggedPtr`, which is useful within
/// structs whose size must be minimized.
/// - Because of the uniqueness assumption, we can use the address of a
/// `List<T>` for faster equality comparisons and hashing.
/// - `T` must be `Copy`. This lets `List<T>` be stored in a dropless arena and
/// iterators return a `T` rather than a `&T`.
/// - `T` must not be zero-sized.
#[repr(C)]
pub struct List<T> {
len: usize,

/// Although this claims to be a zero-length array, in practice `len`
/// elements are actually present.
data: [T; 0],

opaque: OpaqueListContents,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To expand on what I mentioned on Zulip, note here that data and opaque are deeply intertwined; having them laid out in such a bare fashion is thus an itsy bit error-prone. That is, compare the current definition, even with your great comments added to it, to:

#[repr(C)]
pub struct List<T> {
    len: usize,

    /// `len` elements are expected to be present.
    data: ThinDst<[T]>,
}

/// Const-assert that a pointer to a `List<T>` is thin.
const _: () = {
    let _ = ::core::mem::transmute::<&List<u8>, usize>;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you happy with this part of the code as is, or are you suggesting that I change it to what's in the Playground link?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't claim which one is better in a vacuum: I like my suggestion since it reads quite well at the level of the definition of List<T>, but now it's the definition of ThinDst which may seem a bit too "messy" for compiler code. So just let it be, unless other people were to chime in in defense of the ThinDst helper


unsafe impl<'a, T: 'a> rustc_data_structures::tagged_ptr::Pointer for &'a List<T> {
const BITS: usize = std::mem::align_of::<usize>().trailing_zeros() as usize;
#[inline]
fn into_usize(self) -> usize {
self as *const List<T> as usize
}
#[inline]
unsafe fn from_usize(ptr: usize) -> Self {
&*(ptr as *const List<T>)
}
unsafe fn with_ref<R, F: FnOnce(&Self) -> R>(ptr: usize, f: F) -> R {
// Self: Copy so this is fine
let ptr = Self::from_usize(ptr);
f(&ptr)
}
extern "C" {
/// A dummy type used to force `List` to be unsized while not requiring
/// references to it be wide pointers.
type OpaqueListContents;
}

unsafe impl<T: Sync> Sync for List<T> {}
impl<T> List<T> {
/// Returns a reference to the (unique, static) empty list.
#[inline(always)]
pub fn empty<'a>() -> &'a List<T> {
#[repr(align(64))]
struct MaxAlign;

assert!(mem::align_of::<T>() <= mem::align_of::<MaxAlign>());

#[repr(C)]
struct InOrder<T, U>(T, U);

// The empty slice is static and contains a single `0` usize (for the
// length) that is 64-byte aligned, thus featuring the necessary
// trailing padding for elements with up to 64-byte alignment.
static EMPTY_SLICE: InOrder<usize, MaxAlign> = InOrder(0, MaxAlign);
unsafe { &*(&EMPTY_SLICE as *const _ as *const List<T>) }
}
}

impl<T: Copy> List<T> {
/// Allocates a list from `arena` and copies the contents of `slice` into it.
///
/// WARNING: the contents *must be unique*, such that no list with these
/// contents has been previously created. If not, operations such as `eq`
/// and `hash` might give incorrect results.
///
/// Panics if `T` is `Drop`, or `T` is zero-sized, or the slice is empty
nnethercote marked this conversation as resolved.
Show resolved Hide resolved
/// (because the empty list exists statically, and is available via
/// `empty()`).
nnethercote marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
pub(super) fn from_arena<'tcx>(arena: &'tcx Arena<'tcx>, slice: &[T]) -> &'tcx List<T> {
nnethercote marked this conversation as resolved.
Show resolved Hide resolved
assert!(!mem::needs_drop::<T>());
Expand All @@ -73,7 +91,7 @@ impl<T: Copy> List<T> {
.cast::<T>()
.copy_from_nonoverlapping(slice.as_ptr(), slice.len());

&mut *mem
&*mem
}
}

Expand Down Expand Up @@ -107,11 +125,24 @@ impl<S: Encoder, T: Encodable<S>> Encodable<S> for &List<T> {
}
}

impl<T: PartialEq> PartialEq for List<T> {
#[inline]
fn eq(&self, other: &List<T>) -> bool {
// Pointer equality implies list equality (due to the unique contents
// assumption).
ptr::eq(self, other)
}
}

impl<T: Eq> Eq for List<T> {}

impl<T> Ord for List<T>
where
T: Ord,
{
fn cmp(&self, other: &List<T>) -> Ordering {
// Pointer equality implies list equality (due to the unique contents
// assumption), but the contents must be compared otherwise.
if self == other { Ordering::Equal } else { <[T] as Ord>::cmp(&**self, &**other) }
}
}
Expand All @@ -121,6 +152,8 @@ where
T: PartialOrd,
{
fn partial_cmp(&self, other: &List<T>) -> Option<Ordering> {
// Pointer equality implies list equality (due to the unique contents
// assumption), but the contents must be compared otherwise.
if self == other {
Some(Ordering::Equal)
} else {
Expand All @@ -129,17 +162,11 @@ where
}
}

impl<T: PartialEq> PartialEq for List<T> {
#[inline]
fn eq(&self, other: &List<T>) -> bool {
ptr::eq(self, other)
}
}
impl<T: Eq> Eq for List<T> {}

impl<T> Hash for List<T> {
#[inline]
fn hash<H: Hasher>(&self, s: &mut H) {
// Pointer hashing is sufficient (due to the unique contents
// assumption).
(self as *const List<T>).hash(s)
}
}
Expand Down Expand Up @@ -168,13 +195,24 @@ impl<'a, T: Copy> IntoIterator for &'a List<T> {
}
}

impl<T> List<T> {
#[inline(always)]
pub fn empty<'a>() -> &'a List<T> {
#[repr(align(64), C)]
struct EmptySlice([u8; 64]);
static EMPTY_SLICE: EmptySlice = EmptySlice([0; 64]);
assert!(mem::align_of::<T>() <= 64);
unsafe { &*(&EMPTY_SLICE as *const _ as *const List<T>) }
unsafe impl<T: Sync> Sync for List<T> {}

unsafe impl<'a, T: 'a> rustc_data_structures::tagged_ptr::Pointer for &'a List<T> {
const BITS: usize = std::mem::align_of::<usize>().trailing_zeros() as usize;

#[inline]
fn into_usize(self) -> usize {
self as *const List<T> as usize
}

#[inline]
unsafe fn from_usize(ptr: usize) -> &'a List<T> {
&*(ptr as *const List<T>)
}

unsafe fn with_ref<R, F: FnOnce(&Self) -> R>(ptr: usize, f: F) -> R {
// `Self` is `&'a List<T>` which impls `Copy`, so this is fine.
let ptr = Self::from_usize(ptr);
f(&ptr)
}
}