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

Allow passing index as :ident in set_general_handler macro #384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asoderman
Copy link

Hello, please consider this small QOL change to the set_general_handler macro.

This would allow the index to be specified by a const or variable e.g.

const PAGEFAULT: u8 = 0xe;

set_general_handler(idt, page_fault, PAGEFAULT);

instead of being forced to convert it to a range

set_general_handler(idt, page_fault, PAGEFAULT..PAGEFAULT)

I'm not sure if there's any way to make this change less repetitive by having $idx be either literal or ident. It might also be worth considering refactoring out the setting individual handler functionality (the $idx variants) into its own macro and allow users to pass a $range:ident to the current macro to avoid ambiguity.

Copy link
Contributor

@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.

Thanks for this pr and sorry for not reviewing this earlier!

The problem you want to solve with this seems reasonable, but I believe the current changes would introduce a breaking change and I'd like to avoid that if possible.

Comment on lines +1173 to +1175
($idt:expr, $handler:ident, $idx:ident) => {
$crate::set_general_handler!($idt, $handler, $idx..=$idx);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This match arm would overlap with match arm just below this, wouldn't it? We can match literals in the case above this because there's now way to construct a range from a single literal, so a literal always has to be a single index, but the same is not true for idents which can be individual indices as well as ranges.

@Freax13
Copy link
Contributor

Freax13 commented Jun 14, 2022

Perhaps we could approach this from another angle: The SliceIndex trait is used in the standard library to allow indexing with multiple different types. This was already considered in #95 (comment) and later ruled out in #319 because SliceIndex is based on usize and not u8.
Instead, perhaps we could implement our own trait InterruptDescriptorTableIndex that would be implemented on u8 as well as all the range types and use that type for the bounds checks in the macro. The same trait could probably be used for InterruptDescriptorTable::slice as well as InterruptDescriptorTable::index.

@Freax13 Freax13 added the waiting-on-author Waiting for the author to act on review feedback. label Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting for the author to act on review feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants