Skip to content

Conversation

@ahmedcharles
Copy link
Contributor

No description provided.

@4e554c4c
Copy link

This looks like quite a lot of information to be printing by default. Is there any reason that you want to implement fmt::Display for this instead of fmt::Debug?

@ahmedcharles
Copy link
Contributor Author

I'd be fine with implementing Debug instead of Display.

@phil-opp
Copy link
Member

Yeah, I agree that Debug would be better here. Otherwise it looks good to me.

@4e554c4c
Copy link

Hmm, this also seems to suffer from the "won't work on higher half kernels" problem that #14 had. I'm not quite sure how to fix that though, because fmt traits don't take arguments.

@ahmedcharles
Copy link
Contributor Author

Yes, I know that this doesn't work with higher half kernels. The solution in #14 requires unsafe code and that the user of the BootInformation (or some subset of it) know what the offset is. I'd prefer to keep knowledge of the kernel offset to be as limited as possible.

The only way to achieve that with this crate is so store the address and offset, then use the offset whenever an absolute address is dereferenced. I'm going to change my local copy of this to do that soon and I'll open a PR to get opinions when it's working well.

@ahmedcharles
Copy link
Contributor Author

Updated it to impl Debug.

@phil-opp
Copy link
Member

The only way to achieve that with this crate is so store the address and offset, then use the offset whenever an absolute address is dereferenced.

Sounds good to me.

@phil-opp phil-opp changed the title Add Display for BootInformation. Implement Debug for BootInformation Feb 16, 2017
@phil-opp phil-opp merged commit 467a63b into rust-osdev:master Feb 16, 2017
@phil-opp
Copy link
Member

Thanks a lot, @ahmedcharles!

@ahmedcharles ahmedcharles deleted the display branch February 16, 2017 20:12
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.

3 participants