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

Address lint ambiguous_wide_pointer_comparisons #478

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jamesmunns
Copy link
Collaborator

Noticed this warning:

warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> /Users/james/personal/mycelium/cordyceps/src/list.rs:497:12
    |
497 |         if head == tail {
    |            ^^^^^^^^^^^^
    |
    = note: `#[warn(ambiguous_wide_pointer_comparisons)]` on by default
help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
497 |         if std::ptr::addr_eq(head.as_ptr(), tail.as_ptr()) {
    |            ++++++++++++++++++    ~~~~~~~~~~     ++++++++++
help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
497 |         if std::ptr::eq(head.as_ptr(), tail.as_ptr()) {
    |            +++++++++++++    ~~~~~~~~~~     ++++++++++

warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> /Users/james/personal/mycelium/cordyceps/src/list.rs:966:13
    |
966 |             splice_start == splice_end
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
966 |             std::ptr::addr_eq(splice_start.as_ptr(), splice_end.as_ptr())
    |             ++++++++++++++++++            ~~~~~~~~~~           ++++++++++
help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
966 |             std::ptr::eq(splice_start.as_ptr(), splice_end.as_ptr())
    |             +++++++++++++            ~~~~~~~~~~           ++++++++++
    ```

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This is a MSRV bump, since core::ptr::addr_eq was added in Rust 1.76.0, and cordyceps currently claims a minimum Rust version of 1.61.0.

I'm not super fussed about taking the MSRV bump, but if we do, it would be good to make sure to change the Cargo.toml rust-version field as part of this commit, as well.

Alternatively, we could perform the same address-without-metadata comparison by writing

head.as_ptr() as *const () == tail.as_ptr() as *const ()

and

splice_start.as_ptr() as *const () == splice_end.as_ptr() as *const ()

which is basically what ptr::addr_eq does internally, but is permitted on older Rust versions. If we go that route, we can avoid the MSRV bump, but it might be worth adding a comment that the as *const () casts are to ignore the wide pointer metadata, so that a future reader doesn't change them back...

@hawkw
Copy link
Owner

hawkw commented Jul 7, 2024

Ugh, it looks like some of the maitake tests are not feature flagged correctly: https://github.com/hawkw/mycelium/actions/runs/9821097757/job/27116399920?pr=478

I can fix that separately...

@jamesmunns
Copy link
Collaborator Author

Updated MSRV of cordyceps, and all crates that I could see that depend on cordyceps.

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Some smallish nits. I'd like to merge this, though!

@@ -17,7 +17,7 @@ categories = [
"concurrency"
]
edition = "2021"
rust-version = "1.61.0"
rust-version = "1.76.0"
Copy link
Owner

Choose a reason for hiding this comment

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

I don't actually think we need to do the MSRV bump for maitake-sync. Since the cordyceps changes will be released in a semver-compatible version, and maitake-sync's minimum cordyceps version isn't changing, users of maitake-sync can continue using the current cordyceps version rather than picking up the one with the new MSRV --- either by checking in a lockfile, or by explicitly pinning the cordyceps version in their Cargo.toml.

IMHO, we would only need to change maitake-sync's MSRV if we were changing the dependency spec for cordyceps to not permit versions that compile on 1.61.0.

@@ -494,7 +494,7 @@ impl<T: Linked<Links<T>> + ?Sized> List<T> {
let tail_links = unsafe { T::links(tail) };
let head_links = unsafe { head_links.as_ref() };
let tail_links = unsafe { tail_links.as_ref() };
if head == tail {
if core::ptr::addr_eq(head.as_ptr(), tail.as_ptr()) {
Copy link
Owner

Choose a reason for hiding this comment

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

nit, take it or leave it: core::ptr is already imported in this file:

ptr::{self, NonNull},

so we could drop the core prefix:

Suggested change
if core::ptr::addr_eq(head.as_ptr(), tail.as_ptr()) {
if ptr::addr_eq(head.as_ptr(), tail.as_ptr()) {

@@ -963,7 +963,7 @@ impl<T: Linked<Links<T>> + ?Sized> List<T> {
let start_links = T::links(splice_start).as_mut();
let end_links = T::links(splice_end).as_mut();
debug_assert!(
splice_start == splice_end
core::ptr::addr_eq(splice_start.as_ptr(), splice_end.as_ptr())
Copy link
Owner

Choose a reason for hiding this comment

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

similarly, we could write:

Suggested change
core::ptr::addr_eq(splice_start.as_ptr(), splice_end.as_ptr())
ptr::addr_eq(splice_start.as_ptr(), splice_end.as_ptr())

@@ -17,7 +17,7 @@ categories = [
"async",
]
edition = "2021"
rust-version = "1.61.0"
rust-version = "1.76.0"
Copy link
Owner

Choose a reason for hiding this comment

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

again, i think the MSRV bump isn't strictly necessary unless we are also changing the cordyceps dep

@@ -3,7 +3,7 @@ name = "mycelium-util"
version = "0.1.0"
authors = ["Eliza Weisman <[email protected]>"]
edition = "2021"
rust-version = "1.61.0"
rust-version = "1.76.0"
Copy link
Owner

Choose a reason for hiding this comment

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

and again, i don't think this is really necessary...although it's also fine since this isn't a published crate.

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