Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@cecton
Copy link
Contributor

@cecton cecton commented May 7, 2020

No description provided.

cecton added 2 commits May 7, 2020 13:56
Forked at: 3acb6d0
Parent branch: origin/master
@cecton cecton requested review from bkchr and coriolinus May 7, 2020 11:56
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

I think I understand what this is doing, but it looks like a self-contained change; it's not obvious to me why adding the storage_append method fixes things.

@cecton
Copy link
Contributor Author

cecton commented May 7, 2020

It's one of the method of the runtime. If the method is not declared, it falls back to the default implementation that just panics. Literally a runtime exception XD

@coriolinus
Copy link
Contributor

@bkchr Is there any reason not to merge this immediately?

let mut value_vec = Vec::with_capacity(1);
value_vec.push(EncodeOpaqueValue(value));

if let Some(Some(item)) = self.overlay.remove(&key_vec) {
Copy link
Member

Choose a reason for hiding this comment

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

You want to use the entry api here.

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 don't think I want this time. I'm doing a remove to extract the value to transform it and re-insert it in the hashmap. I don't need a mutable reference here

Copy link
Member

Choose a reason for hiding this comment

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

You want it ;) sp_std::mem::take is your friend.

Copy link
Contributor Author

@cecton cecton May 8, 2020

Choose a reason for hiding this comment

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

Probably fixed in here: #92 (comment)

@bkchr
Copy link
Member

bkchr commented May 7, 2020

@bkchr Is there any reason not to merge this immediately?

Sometimes even I take some time to review prs :D

@coriolinus coriolinus mentioned this pull request May 7, 2020
Comment on lines 237 to 249
fn storage_append(&mut self, key: &[u8], value: Vec<u8>) {
let value_vec = sp_std::vec![EncodeOpaqueValue(value)];
let current_value = self.overlay.entry(key.to_vec()).or_default();

if let Some(item) = sp_std::mem::take(current_value) {
*current_value = Some(match Vec::<EncodeOpaqueValue>::append_or_new(item, &value_vec) {
Ok(item) => item,
Err(_) => value_vec.encode(),
});
} else {
*current_value = Some(value_vec.encode());
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation with the entry api instead of remove/insert. Is it good? 😅 I guess since it doesn't remove and then re-insert, this saves an allocation

@bkchr bkchr merged commit e810cf3 into master May 8, 2020
@bkchr bkchr deleted the cecton-add-storage-append branch May 8, 2020 21:54
Maharacha pushed a commit to Maharacha/cumulus that referenced this pull request May 10, 2023
Check if the container is running and finished syncing the parachain
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants