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.

Sign up for GitHub

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

Jump to bottom

make Cr2::read return a result #335

Merged
merged 3 commits into from
Feb 6, 2022
Merged

make Cr2::read return a result #335

merged 3 commits into from
Feb 6, 2022

Conversation

Freax13
Copy link
Member

@Freax13 Freax13 commented Jan 16, 2022 •
edited by phil-opp
Loading

This pr adds Cr2::read_raw and makes Cr2::read return a result.

Closes #332

@Freax13 Freax13 requested a review from phil-opp January 16, 2022 14:07
@Freax13
Copy link
Member Author

Freax13 commented Jan 16, 2022

CI is failing because of the nightly breakages fixed by #329.

phil-opp
phil-opp approved these changes Jan 17, 2022
View reviewed changes
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.

Looks good to me, thanks!

CI is failing because of the nightly breakages fixed by #329.

We should merge master into next then.

src/registers/control.rs Outdated Show resolved Hide resolved
src/registers/control.rs Show resolved Hide resolved
@phil-opp
Copy link
Member

I just saw that the read_raw change is also included in #334. Then we could merge #334 first, then merge master into next, and then rebase this PR so that only the second commit is needed ("make Cr2::read return a result").

@josephlr josephlr mentioned this pull request Jan 18, 2022
Closed
@Freax13
Copy link
Member Author

Freax13 commented Jan 28, 2022

Then we could merge #334 first, then merge master into next, and then rebase this PR so that only the second commit is needed ("make Cr2::read return a result").

Is there a good workflow to merge master into next that maybe also includes someone reviewing the merge? There have been quite a lot of changes and they will be a lot of conflicts.

@phil-opp
Copy link
Member

phil-opp commented Feb 3, 2022

Is there a good workflow to merge master into next that maybe also includes someone reviewing the merge? There have been quite a lot of changes and they will be a lot of conflicts.

I opened a PR to do the merge at #338. Perhaps you could do a quick check that I didn't mess someting up?

@Freax13
Copy link
Member Author

Freax13 commented Feb 3, 2022

I opened a PR to do the merge at #338. Perhaps you could do a quick check that I didn't mess someting up?

Sure, I'll take a look at this later today!

@Freax13 @toku-sa-n
add errors section in doc comments
3de203e 
Co-authored-by: Hiroki Tokunaga <tokusan441@gmail.com>
@Freax13 Freax13 linked an issue Feb 5, 2022 that may be closed by this pull request
Call to VirtAddr::new() causes panic when CR2 does not contain a canonical address. #332
Closed
@Freax13 Freax13 mentioned this pull request Feb 5, 2022
Merged
phil-opp
phil-opp approved these changes Feb 5, 2022
View reviewed changes
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.

Looks good, thanks!

src/registers/control.rs Outdated Show resolved Hide resolved
@phil-opp phil-opp merged commit 628b913 into rust-osdev:next Feb 6, 2022
@Freax13 Freax13 deleted the cr2-read-returns-result branch February 6, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-opp phil-opp phil-opp approved these changes

@toku-sa-n toku-sa-n toku-sa-n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Call to VirtAddr::new() causes panic when CR2 does not contain a canonical address.
3 participants
@Freax13 @phil-opp @toku-sa-n