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

TryFrom implementation for ExceptionVector #506

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

mrjbom
Copy link
Contributor

@mrjbom mrjbom commented Oct 20, 2024

There is currently no way to create an instance of idt::ExceptionVector having an interrupt number. At the same time it would be convenient for the user to use this enum to handle interrupts.

As for the implementation, I couldn't think of what type of error it should return.

@Freax13
Copy link
Member

Freax13 commented Oct 21, 2024

Thank you for your contribution!

There is currently no way to create an instance of idt::ExceptionVector having an interrupt number. At the same time it would be convenient for the user to use this enum to handle interrupts.

As for the implementation, I couldn't think of what type of error it should return.

Let's use a simple error type that wraps the invalid u8. Feel free to use PcidTooBig as inspiration:

/// A passed `u16` was not a valid PCID.
///
/// A PCID has to be <= 4096 for x86_64.
#[derive(Debug)]
pub struct PcidTooBig(u16);
impl fmt::Display for PcidTooBig {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "PCID should be < 4096, got {}", self.0)
}
}

@mrjbom mrjbom force-pushed the TryFrom_for_ExceptionVector branch 2 times, most recently from 1a3495b to 951cf44 Compare October 21, 2024 19:28
@mrjbom
Copy link
Contributor Author

mrjbom commented Oct 21, 2024

Made Error as enum for more details

@mrjbom mrjbom force-pushed the TryFrom_for_ExceptionVector branch from 951cf44 to 587b2fa Compare October 21, 2024 19:56
Comment on lines 1345 to 1373
impl fmt::Display for InvalidExceptionVectorNumber {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
InvalidExceptionVectorNumber::IsCoprocessorSegmentOverrun(exception_vector_number) => {
write!(f, "{exception_vector_number} cannot be a valid exception vector number because the Coprocessor Segment Overrun exception is handled as part of the General Protection Fault")
}
InvalidExceptionVectorNumber::Reserved(exception_vector_number) => {
write!(f, "{exception_vector_number} cannot be a valid exception vector number because it is reserved")
}
InvalidExceptionVectorNumber::NotException(exception_vector_number) => {
write!(f, "{exception_vector_number} cannot be a valid exception vector number because only the first 32 numbers can be exception numbers")
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is certainly a matter of opinion, but the error messages seem too lengthy to me. Let's just print "{} is not a valid exception vector" in all cases and let's also use a struct instead of an enum. It shouldn't be difficult for the user to figure out what went wrong in each case.

@mrjbom mrjbom force-pushed the TryFrom_for_ExceptionVector branch from 587b2fa to bc9441d Compare October 22, 2024 16:49
Copy link
Member

@Freax13 Freax13 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! Sorry for the delay.

@Freax13 Freax13 force-pushed the TryFrom_for_ExceptionVector branch from bc9441d to 6e0652f Compare November 16, 2024 11:47
@Freax13
Copy link
Member

Freax13 commented Nov 16, 2024

I rebased this PR onto the lastest master branch to fix CI.

@Freax13 Freax13 merged commit 08e0172 into rust-osdev:master Nov 16, 2024
12 of 13 checks passed
@mrjbom mrjbom deleted the TryFrom_for_ExceptionVector branch November 18, 2024 02:21
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