Skip to content

react: Distinguish cell IDs #499

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

Merged
merged 3 commits into from
Apr 20, 2018
Merged

react: Distinguish cell IDs #499

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52e622d
react: Distinguish cell IDs
petertseng Mar 17, 2018
a1cf22d
react: Use newtype instead of typedef
petertseng Apr 18, 2018
169ea90
react: set_value: Return bool
petertseng Apr 17, 2018
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
36 changes: 23 additions & 13 deletions exercises/react/example.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::collections::HashMap;

/// `InputCellID` is a unique identifier for an input cell.
pub type InputCellID = usize;
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct InputCellID(usize);
/// `ComputeCellID` is a unique identifier for a compute cell.
/// Values of type `InputCellID` and `ComputeCellID` should not be mutually assignable,
/// demonstrated by the following tests:
Expand All @@ -16,8 +17,10 @@ pub type InputCellID = usize;
/// let input = r.create_input(111);
/// let compute: react::InputCellID = r.create_compute(&[react::CellID::Input(input)], |_| 222).unwrap();
/// ```
pub type ComputeCellID = usize;
pub type CallbackID = usize;
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct ComputeCellID(usize);
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct CallbackID(usize);

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum CellID {
Expand Down Expand Up @@ -90,23 +93,23 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {

pub fn create_input(&mut self, initial: T) -> InputCellID {
self.inputs.push(Cell::new(initial));
self.inputs.len() - 1
InputCellID(self.inputs.len() - 1)
}

pub fn create_compute<F: Fn(&[T]) -> T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<ComputeCellID, CellID> {
// Check all dependencies' validity before modifying any of them,
// so that we don't perform an incorrect partial write.
for &dep in dependencies {
match dep {
CellID::Input(id) => if id >= self.inputs.len() { return Err(dep) },
CellID::Compute(id) => if id >= self.computes.len() { return Err(dep) },
CellID::Input(InputCellID(id)) => if id >= self.inputs.len() { return Err(dep) },
CellID::Compute(ComputeCellID(id)) => if id >= self.computes.len() { return Err(dep) },
}
}
let new_id = self.computes.len();
let new_id = ComputeCellID(self.computes.len());
for &dep in dependencies {
match dep {
CellID::Input(id) => self.inputs[id].dependents.push(new_id),
CellID::Compute(id) => self.computes[id].cell.dependents.push(new_id),
CellID::Input(InputCellID(id)) => self.inputs[id].dependents.push(new_id),
CellID::Compute(ComputeCellID(id)) => self.computes[id].cell.dependents.push(new_id),
}
}
let inputs: Vec<_> = dependencies.iter().map(|&id| self.value(id).unwrap()).collect();
Expand All @@ -117,12 +120,13 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {

pub fn value(&self, id: CellID) -> Option<T> {
match id {
CellID::Input(id) => self.inputs.get(id).map(|c| c.value),
CellID::Compute(id) => self.computes.get(id).map(|c| c.cell.value),
CellID::Input(InputCellID(id)) => self.inputs.get(id).map(|c| c.value),
CellID::Compute(ComputeCellID(id)) => self.computes.get(id).map(|c| c.cell.value),
}
}

pub fn set_value(&mut self, id: InputCellID, new_value: T) -> Result<(), SetValueError> {
let InputCellID(id) = id;
match self.inputs.get_mut(id) {
Some(c) => {
c.value = new_value;
Expand All @@ -142,14 +146,17 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
}

pub fn add_callback<F: FnMut(T) -> () + 'a>(&mut self, id: ComputeCellID, callback: F) -> Option<CallbackID> {
let ComputeCellID(id) = id;
self.computes.get_mut(id).map(|c| {
c.callbacks_issued += 1;
c.callbacks.insert(c.callbacks_issued, Box::new(callback));
c.callbacks_issued
let cbid = CallbackID(c.callbacks_issued);
c.callbacks.insert(cbid, Box::new(callback));
cbid
})
}

pub fn remove_callback(&mut self, cell: ComputeCellID, callback: CallbackID) -> Result<(), RemoveCallbackError> {
let ComputeCellID(cell) = cell;
match self.computes.get_mut(cell) {
Some(c) => match c.callbacks.remove(&callback) {
Some(_) => Ok(()),
Expand All @@ -160,6 +167,8 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
}

fn update_dependent(&mut self, id: ComputeCellID) {
let ComputeCellID(id) = id;

let (new_value, dependents) = {
// This block limits the scope of the self.cells borrow.
// This is necessary becaue we borrow it mutably below.
Expand Down Expand Up @@ -189,6 +198,7 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
}

fn fire_callbacks(&mut self, id: ComputeCellID) {
let ComputeCellID(id) = id;
let dependents = match self.computes.get_mut(id) {
Some(c) => {
if c.cell.value == c.cell.last_value {
Expand Down
11 changes: 6 additions & 5 deletions exercises/react/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Because these are passed without & to some functions,
// it will probably be necessary for these three types to be Copy.
/// `InputCellID` is a unique identifier for an input cell.
pub type InputCellID = ();
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct InputCellID();
/// `ComputeCellID` is a unique identifier for a compute cell.
/// Values of type `InputCellID` and `ComputeCellID` should not be mutually assignable,
/// demonstrated by the following tests:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if cargo doc flags to the user that the code under test here should fail to compile. If it does, no changes are required. If it does not, it's probably worth mentioning in the text that both of the examples here should fail to compile.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did manage to confirm this. There's a red ⓘ and hovering over it shows "this example deliberately fails to compile"

Given this, I'll be merging it. Just not now, since it is inconvenient to now.

Expand All @@ -16,8 +15,10 @@ pub type InputCellID = ();
/// let input = r.create_input(111);
/// let compute: react::InputCellID = r.create_compute(&[react::CellID::Input(input)], |_| 222).unwrap();
/// ```
pub type ComputeCellID = ();
pub type CallbackID = ();
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct ComputeCellID();
#[derive(Clone, Copy, Debug, PartialEq)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the tests are going to need all of these. The reason why Debug and PartialEq should be obvious.

The reason for Copy (and therefore why Clone) is:

error[E0382]: use of moved value: `callback`
   --> tests/react.rs:203:52
    |
201 |     assert!(reactor.remove_callback(output, callback).is_ok());
    |                                             -------- value moved here
202 |     for _ in 1..5 {
203 |         assert_eq!(reactor.remove_callback(output, callback), Err(RemoveCallbackError::NonexistentCallback));
    |                                                    ^^^^^^^^ value used here after move
    |
    = note: move occurs because `callback` has type `react::CallbackID`, which does not implement the `Copy` trait

pub struct CallbackID();

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum CellID {
Expand Down