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

Add upsert and destroy also to vector #6860

Merged
merged 14 commits into from
Mar 15, 2023
61 changes: 61 additions & 0 deletions aptos-move/framework/aptos-stdlib/sources/simple_map.move
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,38 @@ module aptos_std::simple_map {
vector::push_back(&mut map.data, Element { key, value });
}

public inline fun upsert<Key: store, Value: store>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the tradeoff between this vs having a non-inline function that returns the old values (as Option and Option)?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think key should be drop + store and value should also be store + drop. REturn an Option that is not droppable seems useless in practice. At least, this is what Table looks like now.
  2. This function is so long that seems not worth an inline function.

map: &mut SimpleMap<Key, Value>,
key: Key,
value: Value,
drop: |Key, Value|
) {
let len = std::vector::length(&map.data);
let i = 0;
while (i < len) {
let element = vector::borrow(&mut map.data, i);
if (&element.key == &key) {
break
};
i = i + 1;
};
if (i == len) {
vector::push_back(&mut map.data, Element { key, value });
} else {
let Element {key: _k, value: _v} = vector::swap_remove(&mut map.data, i);
vector::push_back(&mut map.data, Element { key, value});
drop(_k, _v);
}
}

public inline fun destroy<Key: store, Value: store>(
map: SimpleMap<Key, Value>,
d: |Key, Value|
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about developer experience passing along a drop function? Is it weird at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think both functions may just want drop kvs. for undroppable kvs, user should handle them carefully with their own logic since a lambda doesn't help much in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for destroy it's fine. I wrote two upserts one with drop lambda, one returning an optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is a lambda not helping?

) {
let SimpleMap { data } = map;
std::vector::destroy(data, |e| { let Element { key, value } = e; d(key, value) });
}

public fun remove<Key: store, Value: store>(
map: &mut SimpleMap<Key, Value>,
key: &Key,
Expand Down Expand Up @@ -158,4 +190,33 @@ module aptos_std::simple_map {

destroy_empty(map);
}

struct OnlyMove has store { val: u64 }

#[test]
public fun upsert_test() {
let map = create<u64, OnlyMove>();
// test adding 3 elements using upsert
upsert(&mut map, 1, OnlyMove { val: 1 }, |_k, _v| { let OnlyMove { val: _ } = _v; });
upsert(&mut map, 2, OnlyMove { val: 2 }, |_k, _v| { let OnlyMove { val: _ } = _v; });
upsert(&mut map, 3, OnlyMove { val: 3 }, |_k, _v| { let OnlyMove { val: _ } = _v; });

assert!(length(&map) == 3, 0);
assert!(contains_key(&map, &1), 1);
assert!(contains_key(&map, &2), 2);
assert!(contains_key(&map, &3), 3);
assert!(borrow(&map, &1).val == 1, 4);
assert!(borrow(&map, &2).val == 2, 5);
assert!(borrow(&map, &3).val == 3, 6);

// change mapping 1->1 to 1->4
upsert(&mut map, 1, OnlyMove { val: 4 }, |_k, _v| { let OnlyMove { val: _ } = _v; });

assert!(length(&map) == 3, 7);
assert!(contains_key(&map, &1), 8);
assert!(borrow(&map, &1).val == 4, 9);

std::debug::print(&b"done");
destroy(map, |_k, _v| { let OnlyMove { val: _ } = _v; });
}
}
13 changes: 13 additions & 0 deletions aptos-move/framework/move-stdlib/sources/vector.move
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,19 @@ module std::vector {
result
}

/// Destroy a vector
public inline fun destroy<Element>(
v: vector<Element>,
d: |Element|
) {
let len = length(&v);
while (len != 0) {
d(pop_back(&mut v));
len = len - 1;
};
destroy_empty(v);
}

// =================================================================
// Module Specification

Expand Down