Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add old rustc support as feature #33

Merged
merged 6 commits into from
Nov 7, 2021
Merged

Add old rustc support as feature #33

merged 6 commits into from
Nov 7, 2021

Conversation

Voultapher
Copy link
Owner

fixes #31

This provides polyfilled support for addr_of_mut for older rustc versions. This required splitting the tests, but has the benefit of sliming down the main test dependencies.

This also documents the version requirements and tests them in the CI.

This provides polyfilled support for addr_of_mut for older rustc versions. This
required splitting the tests, but has the benefit of sliming down the main test
dependencies.
@Voultapher Voultapher force-pushed the old_rust_support branch 2 times, most recently from a8c3c62 to e4ae3dd Compare October 31, 2021 18:02
Also adds old_rust configuration
@Voultapher
Copy link
Owner Author

@mitsuhiko please let me know if this addresses your issue.

@Voultapher Voultapher changed the title Old rust support Add old rustc support as feature Oct 31, 2021
@mitsuhiko
Copy link

@Voultapher would you be open to the idea of adding rustversion as a dependency for old_rust? I'm a bit worried that Rust will start optimizing this and there is then no way to turn off this feature. Other than that, this absolutely would fix my issue.

@Voultapher
Copy link
Owner Author

Good point, this transitively affects library users, who can't control what their users compile with. I'll look at a solution. The most straightforward thing would be to use rustversion with 3 versions of the macro.

@mitsuhiko
Copy link

I believe given how rustversion is structured and the fact that attributes cannot be placed on these macro rules, this is probably the shortest you can get away with

#[doc(hidden)]
#[macro_export]
#[cfg(feature = "old_rust")]
#[rustversion::before(1.51)]
macro_rules! _addr_of_mut {
    ($expr:expr) => {
        &mut $expr as *mut _
    };
}

#[doc(hidden)]
#[macro_export]
#[cfg(feature = "old_rust")]
#[rustversion::since(1.51)]
macro_rules! _addr_of_mut {
    ($expr:expr) => {
        core::ptr::addr_of_mut!($expr)
    };
}

#[doc(hidden)]
#[macro_export]
#[cfg(not(feature = "old_rust"))]
macro_rules! _addr_of_mut {
    ($expr:expr) => {
        core::ptr::addr_of_mut!($expr)
    };
}

@Voultapher
Copy link
Owner Author

Voultapher commented Nov 1, 2021

I just noticed doesn't core::ptr::addr_of_mut!((*joined_ptr.as_ptr()).dependent) here the (*joined_ptr.as_ptr()) already create a reference to uninitialized memory? @steffahn

@Voultapher
Copy link
Owner Author

Referencing https://doc.rust-lang.org/core/ptr/macro.addr_of_mut.html

Note, however, that the expr in addr_of_mut!(expr) is still subject to all the usual rules. In particular, addr_of_mut!(*ptr::null_mut()) is Undefined Behavior because it dereferences a null pointer.

@Voultapher
Copy link
Owner Author

I've pushed a version that implements the current scheme without MaybeUninit, if we conclude that's needed I will update. Note @mitsuhiko your suggested approach didn't work because 'procedural macros cannot expand to macro definitions' at least for rustc 1.36.

@mitsuhiko
Copy link

That looks good!

@Voultapher
Copy link
Owner Author

@mitsuhiko I've posted the question here https://users.rust-lang.org/t/does-addr-of-mut-require-maybeuninit/66754 and hope I can clarify this soon, and then close this and release a new version. Thanks for the patience.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants