Skip to content

Refactor DictKey #2035

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 2 commits into from
Aug 3, 2020
Merged

Refactor DictKey #2035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7eaf57b
cloned() -> copied()
youknowone Jul 27, 2020
67b5d61
Refactor DictKey
youknowone Jul 28, 2020
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
2 changes: 1 addition & 1 deletion common/src/hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub fn hash_float(value: f64) -> PyHash {
x as PyHash * value.signum() as PyHash
}

pub fn hash_value<T: Hash>(data: &T) -> PyHash {
pub fn hash_value<T: Hash + ?Sized>(data: &T) -> PyHash {
let mut hasher = DefaultHasher::new();
data.hash(&mut hasher);
hasher.finish() as PyHash
Expand Down
2 changes: 1 addition & 1 deletion examples/mini_repl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ macro_rules! add_python_function {

// inserts the first function found in the module into the provided scope.
$scope.globals.set_item(
&def.obj_name,
def.obj_name.as_str(),
$vm.context().new_pyfunction(
vm::obj::objcode::PyCode::new(*def.clone()).into_ref(&$vm),
$scope.clone(),
Expand Down
4 changes: 2 additions & 2 deletions vm/src/bytesinner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,9 +883,9 @@ impl PyBytesInner {
return self
.elements
.iter()
.cloned()
.copied()
.filter(|x| *x != b'\t')
.collect::<Vec<u8>>();
.collect();
}

for i in &self.elements {
Expand Down
84 changes: 35 additions & 49 deletions vm/src/dictdatatype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,16 @@ impl<T: Clone> Dict<T> {
}

/// Store a key
pub fn insert<K: DictKey + IntoPyObject + Copy>(
&self,
vm: &VirtualMachine,
key: K,
value: T,
) -> PyResult<()> {
pub fn insert<K>(&self, vm: &VirtualMachine, key: K, value: T) -> PyResult<()>
where
K: DictKey,
{
// This does not need to be accurate so we can take the lock mutiple times.
if self.borrow_value().indices.len() > 2 * self.borrow_value().size {
self.resize();
}
loop {
match self.lookup(vm, key)? {
match self.lookup(vm, &key)? {
LookupResult::Existing(index) => {
let mut inner = self.borrow_value_mut();
// Update existing key
Expand All @@ -166,7 +164,7 @@ impl<T: Clone> Dict<T> {
}
}

pub fn contains<K: DictKey + Copy>(&self, vm: &VirtualMachine, key: K) -> PyResult<bool> {
pub fn contains<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<bool> {
if let LookupResult::Existing(_) = self.lookup(vm, key)? {
Ok(true)
} else {
Expand All @@ -176,7 +174,7 @@ impl<T: Clone> Dict<T> {

/// Retrieve a key
#[cfg_attr(feature = "flame-it", flame("Dict"))]
pub fn get<K: DictKey + Copy>(&self, vm: &VirtualMachine, key: K) -> PyResult<Option<T>> {
pub fn get<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<Option<T>> {
loop {
if let LookupResult::Existing(index) = self.lookup(vm, key)? {
if let Some(entry) = &self.borrow_value().entries[index] {
Expand Down Expand Up @@ -304,7 +302,7 @@ impl<T: Clone> Dict<T> {

/// Lookup the index for the given key.
#[cfg_attr(feature = "flame-it", flame("Dict"))]
fn lookup<K: DictKey + Copy>(&self, vm: &VirtualMachine, key: K) -> PyResult<LookupResult> {
fn lookup<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<LookupResult> {
let hash_value = key.do_hash(vm)?;
let perturb = hash_value;
let mut hash_index: HashIndex = hash_value;
Expand Down Expand Up @@ -359,7 +357,7 @@ impl<T: Clone> Dict<T> {
}

/// Retrieve and delete a key
pub fn pop<K: DictKey + Copy>(&self, vm: &VirtualMachine, key: K) -> PyResult<Option<T>> {
pub fn pop<K: DictKey>(&self, vm: &VirtualMachine, key: &K) -> PyResult<Option<T>> {
loop {
if let LookupResult::Existing(index) = self.lookup(vm, key)? {
let mut inner = self.borrow_value_mut();
Expand Down Expand Up @@ -413,41 +411,41 @@ enum LookupResult {
/// the dictionary. Typical usecases are:
/// - PyObjectRef -> arbitrary python type used as key
/// - str -> string reference used as key, this is often used internally
pub trait DictKey {
fn do_hash(self, vm: &VirtualMachine) -> PyResult<HashValue>;
fn do_is(self, other: &PyObjectRef) -> bool;
fn do_eq(self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool>;
pub trait DictKey: IntoPyObject {
fn do_hash(&self, vm: &VirtualMachine) -> PyResult<HashValue>;
fn do_is(&self, other: &PyObjectRef) -> bool;
fn do_eq(&self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool>;
}

/// Implement trait for PyObjectRef such that we can use python objects
/// to index dictionaries.
impl DictKey for &PyObjectRef {
fn do_hash(self, vm: &VirtualMachine) -> PyResult<HashValue> {
impl DictKey for PyObjectRef {
fn do_hash(&self, vm: &VirtualMachine) -> PyResult<HashValue> {
let raw_hash = vm._hash(self)?;
let mut hasher = DefaultHasher::new();
raw_hash.hash(&mut hasher);
Ok(hasher.finish() as HashValue)
}

fn do_is(self, other: &PyObjectRef) -> bool {
fn do_is(&self, other: &PyObjectRef) -> bool {
self.is(other)
}

fn do_eq(self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
fn do_eq(&self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
vm.identical_or_equal(self, other_key)
}
}

impl DictKey for &PyStringRef {
fn do_hash(self, _vm: &VirtualMachine) -> PyResult<HashValue> {
impl DictKey for PyStringRef {
fn do_hash(&self, _vm: &VirtualMachine) -> PyResult<HashValue> {
Ok(self.hash())
}

fn do_is(self, other: &PyObjectRef) -> bool {
fn do_is(&self, other: &PyObjectRef) -> bool {
self.is(other)
}

fn do_eq(self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
fn do_eq(&self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
if self.is(other_key) {
Ok(true)
} else if let Some(py_str_value) = other_key.payload::<PyString>() {
Expand All @@ -458,49 +456,37 @@ impl DictKey for &PyStringRef {
}
}

// AsRef<str> fit this case but not possible in rust 1.46

/// Implement trait for the str type, so that we can use strings
/// to index dictionaries.
impl DictKey for &str {
fn do_hash(self, _vm: &VirtualMachine) -> PyResult<HashValue> {
fn do_hash(&self, _vm: &VirtualMachine) -> PyResult<HashValue> {
// follow a similar route as the hashing of PyStringRef
let raw_hash = hash::hash_value(&self.to_owned()).to_bigint().unwrap();
let raw_hash = hash::hash_value(*self).to_bigint().unwrap();
let raw_hash = hash::hash_bigint(&raw_hash);
let mut hasher = DefaultHasher::new();
raw_hash.hash(&mut hasher);
Ok(hasher.finish() as HashValue)
}

fn do_is(self, _other: &PyObjectRef) -> bool {
fn do_is(&self, _other: &PyObjectRef) -> bool {
// No matter who the other pyobject is, we are never the same thing, since
// we are a str, not a pyobject.
false
}

fn do_eq(self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
fn do_eq(&self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
if let Some(py_str_value) = other_key.payload::<PyString>() {
Ok(py_str_value.as_str() == self)
Ok(py_str_value.as_str() == *self)
} else {
// Fall back to PyString implementation.
let s = vm.new_str(self.to_owned());
let s = vm.ctx.new_str(*self);
s.do_eq(vm, other_key)
}
}
}

impl DictKey for &String {
fn do_hash(self, vm: &VirtualMachine) -> PyResult<HashValue> {
self.as_str().do_hash(vm)
}

fn do_is(self, other: &PyObjectRef) -> bool {
self.as_str().do_is(other)
}

fn do_eq(self, vm: &VirtualMachine, other_key: &PyObjectRef) -> PyResult<bool> {
self.as_str().do_eq(vm, other_key)
}
}

#[cfg(test)]
mod tests {
use super::{Dict, DictKey, VirtualMachine};
Expand All @@ -513,27 +499,27 @@ mod tests {

let key1 = vm.new_bool(true);
let value1 = vm.new_str("abc".to_owned());
dict.insert(&vm, &key1, value1.clone()).unwrap();
dict.insert(&vm, key1.clone(), value1.clone()).unwrap();
assert_eq!(1, dict.len());

let key2 = vm.new_str("x".to_owned());
let value2 = vm.new_str("def".to_owned());
dict.insert(&vm, &key2, value2.clone()).unwrap();
dict.insert(&vm, key2.clone(), value2.clone()).unwrap();
assert_eq!(2, dict.len());

dict.insert(&vm, &key1, value2.clone()).unwrap();
dict.insert(&vm, key1.clone(), value2.clone()).unwrap();
assert_eq!(2, dict.len());

dict.delete(&vm, &key1).unwrap();
assert_eq!(1, dict.len());

dict.insert(&vm, &key1, value2.clone()).unwrap();
dict.insert(&vm, key1.clone(), value2.clone()).unwrap();
assert_eq!(2, dict.len());

assert_eq!(true, dict.contains(&vm, &key1).unwrap());
assert_eq!(true, dict.contains(&vm, "x").unwrap());
assert_eq!(true, dict.contains(&vm, &"x").unwrap());

let val = dict.get(&vm, "x").unwrap().unwrap();
let val = dict.get(&vm, &"x").unwrap().unwrap();
vm.bool_eq(val, value2)
.expect("retrieved value must be equal to inserted value.");
}
Expand Down
6 changes: 3 additions & 3 deletions vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@ impl ExecutingFrame<'_> {
let idx = self.pop_value();
let obj = self.pop_value();
let value = self.pop_value();
obj.set_item(&idx, value, vm)?;
obj.set_item(idx, value, vm)?;
Ok(None)
}

Expand Down Expand Up @@ -994,12 +994,12 @@ impl ExecutingFrame<'_> {
return Err(vm.new_type_error(msg));
}
}
map_obj.set_item(&key, value, vm).unwrap();
map_obj.set_item(key, value, vm).unwrap();
}
}
} else {
for (key, value) in self.pop_multiple(2 * size).into_iter().tuples() {
map_obj.set_item(&key, value, vm).unwrap();
map_obj.set_item(key, value, vm).unwrap();
}
}

Expand Down
Loading