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

Use BumpPointer::default() #993

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Oct 22, 2023

This PR changes the code snippet in our doc to use BumpPointer::default() to create a new zero-initialized bump pointer struct. It also changes our internal implementation to use BumpPointer::default() instead of BumpPointer::new(zero, zero).

@qinsoon qinsoon requested a review from wks October 22, 2023 23:29
@qinsoon
Copy link
Member Author

qinsoon commented Oct 22, 2023

Hopefully this captures what we discussed on Friday. Feel free to update the PR if I missed anything. @wks

// Copy the bump pointer struct
default_allocator.bump_pointer
};
// Create a fastpath BumpPointer with default(). The BumpPointer from default() will guarantee to fail on the first allocation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we let the binding create BumpPointer with default(), we should document the behaviour of the default() method, noting that we initialise it with 0 and 0 and it will be guaranteed to fail in fast path.

Currently there is a comment inside the BumpPointer::default() method.

    fn default() -> Self {
        // Defaults to 0,0. In this case, the first
        // allocation would naturally fail the check
        // `cursor + size < limit`, and go to the slowpath.
        BumpPointer {
            cursor: Address::ZERO,
            limit: Address::ZERO,
        }
    }

I think we can move the comment into the doc comment (///) of the default() method so that the user knows how the default() method is intended to be used.

It is even better to mention this behaviour in the type-level doc comment of the BumpPointer type, too.

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 have made changes as suggested.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon added this pull request to the merge queue Oct 25, 2023
Merged via the queue into mmtk:master with commit abd38ec Oct 25, 2023
18 of 19 checks passed
@qinsoon qinsoon deleted the bump-pointer-default branch October 25, 2023 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants