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

Deserialize branch #374

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Deserialize branch #374

merged 3 commits into from
Nov 29, 2023

Conversation

richardpringle
Copy link
Contributor

Delegate branch deserialization to the branch-module.

@richardpringle richardpringle force-pushed the move-storable-logic-to-branch branch from 313cae7 to 9fa4827 Compare November 29, 2023 03:10
@richardpringle richardpringle force-pushed the move-storable-logic-to-branch branch from 9fa4827 to 4c9f31b Compare November 29, 2023 04:19
Base automatically changed from move-storable-logic-to-branch to main November 29, 2023 04:38
@richardpringle richardpringle marked this pull request as ready for review November 29, 2023 06:01
Copy link
Collaborator

@rkuris rkuris left a comment

Choose a reason for hiding this comment

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

Approved with nits

@@ -368,96 +372,7 @@ impl Storable for Node {

match meta_raw.as_deref()[TRIE_HASH_LEN + 1].try_into()? {
NodeTypeId::Branch => {
// TODO: add path
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This TODO seems relevant still, but it was deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will go away shortly

offset: addr,
size: ENCODED_CHILD_LEN_SIZE,
})?
.as_deref()[0] as u64;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this will get a clippy exception due to unhandled panic once we add the clippy rules. Maybe:

Suggested change
.as_deref()[0] as u64;
.as_deref().get(0).ok_or(...)? as u64;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 this could also be solved with a macro. We should be casting to an array and using from_be_bytes such that we can make the size any valid numeric type. I'll add a comment to #396

Copy link
Contributor Author

Choose a reason for hiding this comment

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


addr += ENCODED_CHILD_LEN_SIZE as usize;

if len == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: clearer if this and the prior statement were reversed. Perhaps increase addr before setting and testing len.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To go along with your comment about the macro, every time we get a view, we should automatically advance the address. I created #396 to address this


let node = BranchNode {
// TODO: add path
// path: Vec::new().into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Is this comment wrong? This assumes that all branch nodes have no data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will go away shortly


let raw_len = {
let mut buf = [0; DATA_LEN_SIZE];
cursor.read_exact(&mut buf)?;
Copy link
Collaborator

@rkuris rkuris Nov 29, 2023

Choose a reason for hiding this comment

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

nit (DRY): identical code could be refactored into a macro, maybe:

macro_rules! read_data {
    ($cursor:expr, $type:ty) => {{
        let mut buf = [0u8; std::mem::size_of::<$type>()];
        $cursor.read_exact(&mut buf)?;
        <$type>::from_le_bytes(buf)
    }};
}

Then, at the callsite, you can do all the manipulation:

            *child = Some(read_data!(cursor, usize))
                .filter(|addr| *addr != 0)
                .map(DiskAddress::from);

Copy link
Contributor Author

Choose a reason for hiding this comment

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


let node = BranchNode {
// TODO: add path
// path: Vec::new().into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line can be removed with the TODO above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will go away shortly

@richardpringle richardpringle merged commit 31a57de into main Nov 29, 2023
5 checks passed
@richardpringle richardpringle deleted the deserialize-branch branch November 29, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants