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

Switch to performing some checks at compile time #2

Merged
merged 2 commits into from
Apr 28, 2023

Conversation

JonasAlaif
Copy link
Contributor

@JonasAlaif JonasAlaif commented Apr 27, 2023

This PR builds on #1 and introduces three main changes:

  1. Switch BITS from usize to u32, this matches std's assumption that the bit width of pointers (usize) will not exceed 2^23. This originates from LLVM here.
  2. Ensure that mask and alignment are calculated at compile-time by making them an associated constant.
  3. Make use of Allow panicking in constants rust-lang/rfcs#2345 to check the basic alignment vs requested BITS properties at compile time.

Some design decisions:

  • Since the checks from 3. are required to ensure SAFETY (maybe not for the lib, but definitely for the client) I have moved them into PtrImpl (with minimal duplication in fallback)
  • I have kept these checks enabled in fallback since users will want to be able to switch between the two without running into compilation errors.
  • I have kept the "However, if its alignment is not at least 2BITS, this function may panic" behavior disabled for fallback, one may want to actually enable the panic here for the same reason as above.
  • Testing of 3. is no longer so easy - there is no should_panic equivalent for shouldnt_compile. I opted to use compiletest_rs which is commonly used in such situations. Unfortunately it comes with a few issues since one needs to manually link in the tagged_pointer.rlib to the compile-fail tests.
    Due to the dependency on compiletest_rs and this issue, std is enabled for cargo test (only).

@taylordotfish taylordotfish merged commit b2a7511 into taylordotfish:master Apr 28, 2023
@taylordotfish
Copy link
Owner

Thanks, merged. I made a few modifications, mostly related to compatibility:

  • The panics in constants are nice, but to keep compatibility with the older Rust versions that tagged-pointer supports, I modified the compile-time checks to use the array indexing hack instead:

    const ASSERT: bool = {
        ["assertion 1 failed"][!condition1 as usize];
        ["assertion 2 failed"][!condition2 as usize];
        // etc.
        true
    };

    The error messages aren't quite as nice, but everything still gets checked at compile-time.

  • I agree it makes more sense for BITS to be a u32, but this is unfortunately a breaking change. However, I added a type alias to make it easier to change the type in a future breaking version.

  • An alternative to the compiletest-rs tests is to (ab)use doctests:

    /// ```compile_fail
    /// // code that shouldn't compile
    /// ```
    mod test {}

    This isn't as flexible as compiletest-rs, since you can't check error messages (although, to be fair, the old should_panic tests didn't either, even though they could have), but it does have the advantage of not requiring any dependencies, so I've duplicated the compiletest-rs tests as doctests. I've still kept the original compiletest-rs tests around, but I made compiletest_rs an optional dependency—the tests will run when the dependency is enabled.

  • I re-added the wrapping_sub when subtracting offset in PtrImpl::get. Although unlikely, align_offset could theoretically return an offset larger than Self::ALIGNMENT, and we don't want to panic in this case. I also updated the comment to reflect the importance of applying the mask—it's not just for the unlikely case where offset >= Self::ALIGNMENT; it's also crucial in the much more likely case where offset is zero.

(Also, thanks for updating “dereferencable” to “dereferenceable”. I had to do a git blame on Rust's source to make sure I wasn't going crazy—it did used to be spelled “dereferencable”.)

@JonasAlaif
Copy link
Contributor Author

The modifications make a lot of sense and your explanations of them are very helpful, thanks :)
I am not strongly attached to the compiletest_rs stuff that I added, so if you want to remove it to reduce complexity at some point in the future then feel free.
Haha, I only noticed the mismatch when I copied "dereferencable" from your code to ctrl+f for it in core::ptr#safety

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