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

Cleanup const_fn! #255

Merged
merged 2 commits into from
May 14, 2021
Merged

Cleanup const_fn! #255

merged 2 commits into from
May 14, 2021

Conversation

josephlr
Copy link
Contributor

Fixes #222

The following methods can be made const on stable:

  • GlobalDescriptorTable::from_raw_slice
  • MappedFrame::{start_address, size}
  • Page<Size4KiB>::p1_index

The remaining functions still need const_fn because:

  • Some GDT methods use &mut self
  • The IDT uses function pointers as a type argument
  • The PageSize marker trait is used all over

Comments were updated where appropriate.

Signed-off-by: Joe Richey [email protected]

@josephlr josephlr requested a review from phil-opp May 14, 2021 08:56
Fixes #222

The following methods can be made `const` on stable:
  - `GlobalDescriptorTable::from_raw_slice`
  - `MappedFrame::{start_address, size}`
  - `Page<Size4KiB>::p1_index`

The remaining functions still need `const_fn` because:
  - Some GDT methods use `&mut self`
  - The IDT uses function pointers as a type argument
  - The PageSize marker trait is used all over

Comments were updated where appropriate.

Signed-off-by: Joe Richey <[email protected]>
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 great, thanks a lot!

src/structures/paging/page_table.rs Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
Signed-off-by: Joe Richey <[email protected]>
@josephlr josephlr merged commit dd1a4c1 into master May 14, 2021
@josephlr josephlr deleted the constfn branch May 14, 2021 10:06
Comment on lines +120 to +127
#[cfg(feature = "const_fn")]
assert!(
next_free <= 8,
"initializing a GDT from a slice requires it to be **at most** 8 elements."
);
#[cfg(not(feature = "const_fn"))]
table[next_free]; // Will fail if slice.len() > 8

Copy link
Member

Choose a reason for hiding this comment

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

Why can't we use assert! in every case? Also, table[next_free] panics if slice.len() >= 8, not slice.len() > 8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert! doesn't work as const panics are not yet stable. See: rust-lang/rust#85194

Good pont on the off-by-one error though, i'll fix it

Copy link
Member

Choose a reason for hiding this comment

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

@josephlr Did we end up fixing this yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I forgot about this. PR for fix: #269

@toku-sa-n
Copy link
Member

* The IDT uses function pointers as a type argument

I tried to remove the const_fn! macro from InterruptDescriptorTable::new(), added the const attribute, and compiled it successfully, so I can't figure out what it means. Do you mean another function or method?

@josephlr
Copy link
Contributor Author

josephlr commented Jul 7, 2021

I tried to remove the const_fn! macro from InterruptDescriptorTable::new(), added the const attribute, and compiled it successfully, so I can't figure out what it means.

No, that's the right function. Building the crate with: cargo check --no-default-features --features=abi_x86_interrupt with that function unconditionally const fn gives me a compiler error:

error[E0658]: function pointers cannot appear in constant functions
   --> src/structures/idt.rs:389:31
    |
389 |                 divide_error: Entry::missing(),
    |                               ^^^^^^^^^^^^^^^^
    |
    = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
    = help: add `#![feature(const_fn_fn_ptr_basics)]` to the crate attributes to enable

@toku-sa-n
Copy link
Member

Ah, I didn't enable that feature and misunderstood the const_fn! macro. Thank you!

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.

Evaluate const functions for stable
3 participants