bytecodealliance / bytecodealliance/wasmtime

Remove `PackedOption` and change `EntityRef` into generic struct.

Open
#5,026 8 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
18.6k
Forks
1.8k
Avg merge
1d 19h
Merged PRs (30d)
121

Description

#### Feature

Removal of `PackedOption`. Making `EntityRef` a generic struct, where `T` would be the type it is pointing to.

#### Benefit

`PackedOption` is making code verbose and unnatural. This proposal allows using `Option` while preserving advantages of `PackedOption`s compactness. Also eliminates need for `ReservedValue`.

Changing `EntityRef` from trait to generic struct can greatly simplify `cranelift_entity` implementation and save us some names when defining entity data. The structure can also implement all of the useful traits on one place so no shorthand macro is needed.

#### Implementation

Here is a quick demonstration of how this can work. This solution requires (forever) unstable feature.

```rust
#![feature(rustc_attrs)]

use std::{marker::PhantomData, mem, ops::{Index, IndexMut}};

pub struct PrimaryMap {
data: Vec,
}

impl PrimaryMap {
pub fn insert(&mut self, value: T) -> EntityRef {
let index = self.data.len();
self.data.push(value);
EntityRef::new(index as u32).expect("no more entities can be spawned")
}
}

impl Index> for PrimaryMap {
type Output = T;

fn index(&self, index: EntityRef) -> &Self::Output {
&self.data[index.index()]
}
}

impl IndexMut> for PrimaryMap {
fn index_mut(&mut self, index: EntityRef) -> &mut Self::Output {
&mut self.data[index.index()]
}
}

// We might want to choose a different index representation but most common one is
// as default. Also the name is a bit long for something common, `ERef` or `VRef`
// (Virtual Reference) might be a better fit.
#[repr(transparent)]
pub struct EntityRef(R, PhantomData<*const T>);

impl EntityRef {
pub fn new(args: R::InitArgs) -> Option {
R::new(args).map(|r| Self(r, PhantomData))
}

pub fn index(&self) -> usize {
self.0.index()
}

pub fn args(self) -> R::InitArgs {
self.0.args()
}
}

// Making representation as flexible as possible.
pub trait EntityRefRepr: Sized {
type InitArgs;

fn new(args: Self::InitArgs) -> Option;

fn index(&self) -> usize;

fn args(self) -> Self::InitArgs;
}

// Compiler is able to optimize the size
const _: () = assert!(std::mem::size_of::>>() == 4);
// Works even on structures that contain non-optional `EntityRef`
const _: () = assert!(std::mem::size_of::>>() == 8);

// If we cannot use nightly, this solution is basically useless :(
#[rustc_layout_scalar_valid_range_end(4294967294)]
#[repr(transparent)]
pub struct NonMaxU32(u32);

impl NonMaxU32 {
pub fn new(value: u32) -> Option {
unsafe { mem::transmute(Self(value)) } // might not be correct implementation
}

pub fn get(&self) -> u32 {
self.0
}
}

impl EntityRefRepr for NonMaxU32 {
type InitArgs = u32;

fn new(args: Self::InitArgs) -> Option {
Self::new(args)
}

fn index(&self) -> usize {
self.get() as usize
}

fn args(self) -> Self::InitArgs {
self.get()
}
}
```

More extensive demonstration can be found [here](https://github.com/jakubDoka/Catalyst).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.