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

Made hole module public for external uses. #47

Merged
merged 10 commits into from Dec 28, 2020
Merged

Made hole module public for external uses. #47

merged 10 commits into from Dec 28, 2020

Conversation

MarcoCicognani
Copy link
Contributor

Hi Phil!

I've made hole.rs public for external uses and moved align_layout() function into HoleList.

Heap still contains the size and the used counters, so it is still Heap that uses align_layout.
Tell me if this is not the best solution

@phil-opp
Copy link
Member

Thanks!

Heap still contains the size and the used counters, so it is still Heap that uses align_layout.
Tell me if this is not the best solution

I would prefer to move the align_layout calls to HoleList::allocate_first_fit and HoleList::deallocate. Maybe we could use a &mut Layout reference as argument, which is then adjusted. Alternatively, we could also return the adjusted Layout as a second return type. What do you think?

@MarcoCicognani
Copy link
Contributor Author

I think that is a good idea.
I implement it immediately

@MarcoCicognani
Copy link
Contributor Author

But size and used must remain into Heap, right?

@phil-opp
Copy link
Member

phil-opp commented Dec 28, 2020

Yes, I would keep them in Heap, to keep the HoleList as simple as possible.

@MarcoCicognani
Copy link
Contributor Author

Well, done!

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks! A few more nits, then this should be ready.

src/hole.rs Outdated
@@ -75,17 +75,17 @@ impl HoleList {
/// block is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the docs for the new second return type? For example:

"[...] Then the start address of that block and the aligned layout are returned. The automatic layout alignment is required because the HoleList has some additional layout requirements for each memory block."

src/hole.rs Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 99 to 103
let res = self.holes.allocate_first_fit(layout);
if res.is_ok() {
self.used += aligned_layout.size();
self.used += res.unwrap().1.size();
}
res
res.map(|tuple| tuple.0)
Copy link
Member

Choose a reason for hiding this comment

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

You could simplify this with a match to avoid the unwrap():

match self.holes.allocate_first_fit(layout) {
    Ok((ptr, aligned_layout)) => {
        self.used += aligned_layout.size();
        Ok(ptr)
    }
    Err(err) => Err(err),
}

@MarcoCicognani
Copy link
Contributor Author

Done

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thank you!

@phil-opp phil-opp merged commit 2b38898 into rust-osdev:master Dec 28, 2020
@phil-opp phil-opp changed the title Made hole.rs public for external uses. Made hole module public for external uses. Dec 28, 2020
phil-opp added a commit that referenced this pull request Dec 28, 2020
@phil-opp
Copy link
Member

Published as v0.8.10

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.

None yet

2 participants